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

Implement "gap" property for flexbox algorithm #248

Merged
merged 22 commits into from
Nov 23, 2022

Conversation

nicoburns
Copy link
Collaborator

Objective

  • Implements flexbox "gap" property (fixes Implement "gap" property (part of the spec for flex layout) #66)
  • Adds support for CSS grid properties to the test generation script (I had already added gap as part of this work, so it made sense to pull that in for this - it doesn't actually effect the generated tests because the fixtures don't actually include any grid properties).

Context

Yoga have recently implemented this. I pulled in their tests. They don't support percentage gaps, so we may need some more testing around that (or to remove that feature from this implementation).

This builds upon #246, which should be merged first.

@alice-i-cecile alice-i-cecile added this to the 0.2 milestone Nov 22, 2022
@alice-i-cecile alice-i-cecile added the enhancement New feature or request label Nov 22, 2022
@nicoburns
Copy link
Collaborator Author

Update: percentage gaps work fine if the parent node has a defined size, but they don't match Chrome/Firefox if the parent does not have a defined size.

@nicoburns nicoburns force-pushed the flexbox-gap branch 2 times, most recently from 5ec4ca6 to 180b8b2 Compare November 23, 2022 11:16
@nicoburns
Copy link
Collaborator Author

Ok, I think I have percentage gaps with undefined container sizes working properly (matching Chrome/Firefox) now. I have to admit that this was a bit of a trial and error process, and I don't fully understand why it works the way it does. But I'm fairly to happy to merge it anyway as:

  • I have generated tests that cover a few different cases
  • The changes were relatively small, and they don't regress any of the existing tests.
  • I can't imagine anyone actually wanting to use percentage gaps with undefined container sizes: the result is that the children overflow the container. I imagine that the vast majority of people will use fixed gaps AND also defined container sizes (i.e. they will avoid this code path in two separate ways).

This PR is now ready for review (although in practice is might be best to wait until to #246 is merged in order to get a better diff).

@alice-i-cecile
Copy link
Collaborator

#246 is merged! Can you rebase so I can review without losing my mind?

@nicoburns nicoburns changed the base branch from main to jk/abstract-over-storage November 23, 2022 15:27
@nicoburns nicoburns changed the base branch from jk/abstract-over-storage to main November 23, 2022 15:27
@nicoburns
Copy link
Collaborator Author

@alice-i-cecile rebased :)

@alice-i-cecile
Copy link
Collaborator

I'm very happy with this. It's great to see how far the code base has come. And thank goodness for the generated tests.

I've suggested some comments to help new readers get more comfortable with the "why" of gaps.

@alice-i-cecile
Copy link
Collaborator

Can we add an example demonstrating this?

scripts/gentest/src/main.rs Outdated Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
@Weibye
Copy link
Collaborator

Weibye commented Nov 23, 2022

Also: ❤️ @nicoburns You've put in amazing work here over the last few weeks / days. Really appreciate it!

@nicoburns
Copy link
Collaborator Author

Can we add an example demonstrating this?

Example added.

But unfortunately in running said example, it became obvious that it wasn't computing the results quite correctly. I think this is a regression in the "fixed size gaps with undefined parent size" case due to the changes for the "percentage gaps with undefined parent size". Which wasn't caught because the existing tests because they were only testing with a defined parent size.

I've added another test to cover this case (which is still failing for now).

@alice-i-cecile
Copy link
Collaborator

Not merging until that regression is fixed, but the code itself is in good shape.

@nicoburns
Copy link
Collaborator Author

Regression fixed :)

@alice-i-cecile alice-i-cecile merged commit 9e53f03 into DioxusLabs:main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement "gap" property (part of the spec for flex layout)
3 participants