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.

Sign up for GitHub

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

Jump to bottom

Make padding/border size floor node size #372

Merged
merged 22 commits into from
Mar 9, 2023
Merged

Make padding/border size floor node size #372

merged 22 commits into from
Mar 9, 2023

Conversation

nicoburns
Copy link
Collaborator

Objective

Fix node sizing behaviour where size is/should be determined by border.
(PR currently only contains failing tests)

Context

bevyengine/bevy#7795 (comment)

@alice-i-cecile alice-i-cecile added the bug Something isn't working label Feb 23, 2023
@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch 2 times, most recently from e5866a1 to cec3c2e Compare March 5, 2023 16:29
@nicoburns nicoburns changed the title WIP: Make border size floor node size WIP: Make padding/border size floor node size Mar 5, 2023
@nicoburns
Copy link
Collaborator Author

nicoburns commented Mar 5, 2023 •
edited
Loading

Some notes:

  • The sum of border and padding should floor the size of the node
  • Like any other min-content size this overrides both size and max-size
  • Aspect-ratio applies to these sizes
  • Need to test for:
    • Leaf
    • Flexbox (container)
    • Flexbox (relative child)
    • Flexbox (absolute child)
    • Grid (container)
    • Grid (child)

The tests in tests/borders_and_padding.rs seem to be enforcing incorrect behaviour here, so I intend to convert those tests to gentests.

@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch 2 times, most recently from 96df489 to 1509bdc Compare March 5, 2023 23:03
@nicoburns nicoburns mentioned this pull request Mar 7, 2023
Merged
@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch from d32afdd to 71adf0c Compare March 8, 2023 19:07
@nicoburns nicoburns changed the title WIP: Make padding/border size floor node size Make padding/border size floor node size Mar 8, 2023
@nicoburns
Copy link
Collaborator Author

I believe this is now ready if we ignore aspect-ratio. Which I'm inclined to do for the time being because I think that's quite a deep rabbit hole to go down and very unlikely combination of styles for people to be using.

@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch 2 times, most recently from 6bcc01b to 46a354c Compare March 9, 2023 16:12
alice-i-cecile
alice-i-cecile reviewed Mar 9, 2023
View reviewed changes
src/compute/leaf.rs Outdated Show resolved Hide resolved
alice-i-cecile
alice-i-cecile requested changes Mar 9, 2023
View reviewed changes
Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

A couple of small quality issues, then this LGTM. We should resolve clippy failures from the new Rust version in their own PR.

@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch from 46a354c to 82aff45 Compare March 9, 2023 17:47
nicoburns added 12 commits March 9, 2023 17:56
@nicoburns
Fix padding/border flooring size of absolutely positioned children
cfa4fec
@nicoburns
Rename grid padding/border tests so that they are all located together
77b56d9
@nicoburns
Make padding/border floor node size for grid children
415959e
@nicoburns
Make padding/border floor node size for grid containers
b44b345
@nicoburns
Apply aspect ratio to leaf nodes whose size is determined by padding/…
c372005 
…border sum
@nicoburns
Add test for padding/border not affecting flex basis
b512668
@nicoburns
Convert flex basis determination to use break from block
171cdea
@nicoburns
Add test for flex-basis 0 with flex grow
cb6c401
@nicoburns
Floor flex-basis limits by preferred size style value when determinin…
39bf6ce 
…g flex container main size
@nicoburns
Floor outer flex-basis by padding_border sum (floors inner flex-basis…
d1b0451 
… at 0)
@nicoburns
Remove commented out code
ef3774d
@nicoburns
Add missing 0
aa251a1
@nicoburns nicoburns force-pushed the fix/border-floors-node-size branch from 82aff45 to aa251a1 Compare March 9, 2023 17:56
@alice-i-cecile alice-i-cecile merged commit c2f99bf into DioxusLabs:main Mar 9, 2023
Weibye
Weibye reviewed Mar 11, 2023
View reviewed changes
Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

Your comments are incredibly insightful and helpful, so keep that up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Weibye Weibye Weibye left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicoburns @alice-i-cecile @Weibye