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

implementing gap property for flexbox #621

Merged

Conversation

ChrisFloofyKitsune
Copy link
Contributor

need to fix offsets growing too large with justify-content use

need to fix flex wrapping not happening properly

need to test with align-items use

@ChrisFloofyKitsune
Copy link
Contributor Author

Having issues with this visual bug popping up...

Without gap changes

image

With gap changes

image

The bottom of the divs in this nested flexbox are getting cut off

@mikke89
Copy link
Owner

mikke89 commented Jun 9, 2024

Nice to see work on this property :)

One way to think about this, is that the gap should behave as if adding extra length to the margins between all the items on the same row/column. Maybe it's possible to make use of the logic to handle margin? It's maybe a bit tricky regardless when it comes to wrapping especially.

We should also make sure to test for any changes/regressions with our visual test suite and the flexbox testes there-in.

need to fix offsets growing too large with justify-content use
need to fix flex wrapping not happening properly
need to test with align-items use
@ChrisFloofyKitsune
Copy link
Contributor Author

Added test cases to the visual tests and...

I am way off 🙃
image

@mikke89
Copy link
Owner

mikke89 commented Jun 10, 2024

Nice, it's a start though! Something visual to work towards :)

@ChrisFloofyKitsune
Copy link
Contributor Author

By adding the gaps as extra margin on the FlexItems, I got it to pass all the visual tests.

Gap adding logic is only run if the gap properties are greater than zero.

@ChrisFloofyKitsune ChrisFloofyKitsune marked this pull request as ready for review June 10, 2024 23:19
Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

I quite like the solution, and it looks correct to me! Nice.

It's a bit hard to understand exactly how it affects (or even how it should affect) the flexible length calculations, in terms of flex base size and such. But I think it makes sense like this, and it is an improvement regardless, so I am happy to take this addition to the algorithm.

I have a few minor comments. Also, the formatting seems to be a bit off, can you run it through clang-format?

Source/Core/Layout/FlexFormattingContext.cpp Outdated Show resolved Hide resolved
Tests/Data/VisualTests/gap-001.rml Outdated Show resolved Hide resolved
Source/Core/Layout/FlexFormattingContext.cpp Outdated Show resolved Hide resolved
@mikke89 mikke89 added enhancement New feature or request layout Layout engine issues and enhancements labels Jun 11, 2024
@ChrisFloofyKitsune
Copy link
Contributor Author

ChrisFloofyKitsune commented Jun 11, 2024

I quite like the solution, and it looks correct to me! Nice.

It's a bit hard to understand exactly how it affects (or even how it should affect) the flexible length calculations, in terms of flex base size and such. But I think it makes sense like this, and it is an improvement regardless, so I am happy to take this addition to the algorithm.

I have a few minor comments. Also, the formatting seems to be a bit off, can you run it through clang-format?

I've cleaned up the issues you raised in the comments

@ChrisFloofyKitsune ChrisFloofyKitsune changed the title initial work on implementing gap property for flexbox implementing gap property for flexbox Jun 12, 2024
@mikke89 mikke89 merged commit b5bf23d into mikke89:master Jun 12, 2024
31 checks passed
@mikke89
Copy link
Owner

mikke89 commented Jun 12, 2024

Looks great, thanks a lot for the PR! This is a very welcome addition. ;) I just made some minor formatting changes at the end here.

mikke89 added a commit to mikke89/RmlUiDoc that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request layout Layout engine issues and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants