Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Center CheatSheet vertically #1985

Merged
merged 44 commits into from
Feb 25, 2025
Merged

Conversation

Ni-g-3l
Copy link
Contributor

@Ni-g-3l Ni-g-3l commented Feb 13, 2025

Hello

I saw yesterday that I can help on some issues, so here is the first one.

This fix allow vertical alignment of CheatSheet

Feel free to comment and or ask me anything to change !

Linked Issue : #1780

Result :
image

@mwestphal
Copy link
Contributor

Sorry I canceled your CI run, working on a CI issue right now

@mwestphal
Copy link
Contributor

I restarted CI :)

@mwestphal
Copy link
Contributor

So regarding the fix, I wonder if the cheatsheet overlay should stay closer to the text instead of extending all the way to the bottom/top ?

Wdyt ?

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 15, 2025

I try to make the cheatsheet stay close to the text and I think it's much cleaner than the other one.
image

I you agree I can commit this change and fix CI too.

@mwestphal
Copy link
Contributor

Looks great to me!

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 15, 2025

I have a question ? Is there a command like : make format or something else, setup to project standard ?

@mwestphal
Copy link
Contributor

make format or something else, setup to project standard ?

clang-format -i /path/to/file for a single file
(shopt -s globstar; clang-format -i **/*.{h,cxx} for the whole project

@mwestphal
Copy link
Contributor

some errors to fix and baselines to update, you can download them from the CI pipeline.

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 22, 2025

Done ! Thanks for the help !

@mwestphal
Copy link
Contributor

none of the new baselines seems different, are you sure these changes are needed ?

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Feb 23, 2025

I think so ! Without updating Baselines CICD no longer pass

@mwestphal
Copy link
Contributor

alright :)

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change needed.

@mwestphal
Copy link
Contributor

@snoyer please review

@snoyer
Copy link
Contributor

snoyer commented Feb 23, 2025

Look good, but did we lose the test where the window is actually taller than the cheatsheet @mwestphal?

@mwestphal
Copy link
Contributor

Look good, but did we lose the test where the window is actually taller than the cheatsheet @mwestphal?

We did, as I want to avoid showing the raytracing stuff in the baselines to avoid duplicating baselines. Since the baseline were still impacted by the change, I guess its fine ?

@snoyer
Copy link
Contributor

snoyer commented Feb 23, 2025

Look good, but did we lose the test where the window is actually taller than the cheatsheet @mwestphal?

We did, as I want to avoid showing the raytracing stuff in the baselines to avoid duplicating baselines. Since the baseline were still impacted by the change, I guess its fine ?

Coverage will be fine but the new feature/behavior isn't tested then? I don't know what would be the best solution here tho, add a test with a tall window but only test it on a single known config (eg. no raytracing)?

@mwestphal
Copy link
Contributor

add a test with a tall window but only test it on a single known config (eg. no raytracing)?

Actually I'd rather add a test that only runs without raytracing.
Easier to update the baseline and the test is still valid and will be run in the mindeps CI.

@Ni-g-3l can you add a test with a long window that only run with raytracing is disabled ?

@Ni-g-3l Ni-g-3l requested a review from mwestphal February 24, 2025 17:22
@mwestphal
Copy link
Contributor

@snoyer please review

@mwestphal mwestphal merged commit 695141f into f3d-app:master Feb 25, 2025
54 checks passed
@mwestphal
Copy link
Contributor

Thanks for your contribution @Ni-g-3l !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants