Skip to content

New CSSBoxWidget and lots of bug fixes #1135

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
Merged
merged 16 commits into from
Sep 17, 2022
Merged

New CSSBoxWidget and lots of bug fixes #1135

merged 16 commits into from
Sep 17, 2022

Conversation

Sub6Resources
Copy link
Owner

@Sub6Resources Sub6Resources commented Sep 6, 2022 •
edited
Loading

There's a lot going on in this branch but it's time for review.

tldr; Calculating widths/margins/padding/border is hard, so I made a new widget that replaces both ContainerSpan and StyledText. This refactor also caused a good chunk of Style code to need refactoring, so I did so, adding support for em, rem, px, %, and auto values on the margin/width/height properties.

Fixes:

  • Fix BRs are not handled correctly in shrinkWrap mode #731, newline
    issue
  • Aligns the baseline of inline content with the baseline of its parent flow, even if it has padding or borders
  • Improved fontSize inheritance
  • Auto margins now work for any block-flow element.
  • auto width and height is now the default, rather than null

Enhancements:

  • New CSSBoxWidget that extends MultiChildRenderObjectWidget to provide specialized layout abilities and access to child sizes during the layout phase.
  • New Margin, Width, and Height classes to allow em, rem, px, auto, and % values to be used. (These extend a new Dimension class that does Unit checking to make sure only valid units are used)
  • Negative margins are now allowed
  • em values now used for h1-6 and p tag default margins and font sizes, which are more accurate at various font sizes.

Notable breaking change:

  • Now requires Dart sdk >= Dart 2.17

@codecov
Copy link

codecov bot commented Sep 6, 2022 •
edited
Loading

Codecov Report

Base: 51.09% // Head: 51.44% // Increases project coverage by +0.35% 🎉

Coverage data is based on head (7581ea7) compared to base (ee9f478).
Patch coverage: 47.96% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
+ Coverage   51.09%   51.44%   +0.35%     
==========================================
  Files          11       17       +6     
  Lines        2018     2587     +569     
==========================================
+ Hits         1031     1331     +300     
- Misses        987     1256     +269
Impacted Files Coverage Δ
lib/src/anchor.dart 66.66% <0.00%> (-13.34%) ⬇️
lib/src/utils.dart 77.27% <0.00%> (+54.54%) ⬆️
lib/src/layout_element.dart 36.47% <4.00%> (-3.78%) ⬇️
lib/src/css_parser.dart 13.48% <4.59%> (-2.80%) ⬇️
lib/src/style/lineheight.dart 11.11% <11.11%> (ø)
lib/src/style/size.dart 33.33% <33.33%> (ø)
lib/src/style/fontsize.dart 56.25% <56.25%> (ø)
lib/style.dart 84.77% <58.62%> (-0.18%) ⬇️
lib/src/css_box_widget.dart 64.85% <64.85%> (ø)
lib/html_parser.dart 72.92% <74.57%> (-0.26%) ⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Sub6Resources Sub6Resources mentioned this pull request Sep 6, 2022
Closed
@erickok
Copy link
Contributor

erickok commented Sep 6, 2022

This is impressive work work Matthew! I am going to take it for a spin tomorrow.

I have a bunch of experience with golden widget tests so if you like I can help with that.

@erickok
Copy link
Contributor

erickok commented Sep 7, 2022

This is working really well. It's not that it solves all issues we had rioght now (and it doesn't need to) but it's a great foundation. I tested on Android and Web and rendering looks good.

The code is looking good, evn though it's a bit tough to read through all changes. But that's okay, I'd rather have this base with a bug than the spaghetti we had right now sometimes. Good job @Sub6Resources

erickok
erickok previously approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@erickok erickok left a comment

Choose a reason for hiding this comment

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

This is amazing work. Consider my comments as just thoughts/considerations/future work, as it's really solid code. Great job!

lib/src/css_box_widget.dart
width = childSize.width + horizontalMargins;
height = childSize.height + verticalMargins;
break;
case Display.LIST_ITEM:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought: This might not be sufficient for list items that (via css) you force to a specific display: inline?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to push this one off as future work. We've got a whole slew of list issues that need to be resolved so I'll tackle this when I start tackling those.

@Sub6Resources
Copy link
Owner Author

Going to go ahead and merge this since the most recent changes were pretty trivial, unit tests passed, and my visual tests look the same before and after.

@Sub6Resources Sub6Resources merged commit ff059b7 into master Sep 17, 2022
@Sub6Resources Sub6Resources deleted the enhancement/margin-values branch September 17, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erickok erickok erickok left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BRs are not handled correctly in shrinkWrap mode
3 participants
@Sub6Resources @erickok @zbarbuto