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

New CSSBoxWidget and lots of bug fixes #1135

Merged
merged 16 commits into from
Sep 17, 2022
Merged

Conversation

Sub6Resources
Copy link
Owner

@Sub6Resources Sub6Resources commented Sep 6, 2022

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

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.

@erickok
Copy link
Collaborator

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
Collaborator

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
Copy link
Collaborator

@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 Outdated Show resolved Hide resolved
lib/src/style/length.dart Outdated Show resolved Hide resolved
packages/flutter_html_table/lib/flutter_html_table.dart Outdated Show resolved Hide resolved
width = childSize.width + horizontalMargins;
height = childSize.height + verticalMargins;
break;
case Display.LIST_ITEM:
Copy link
Collaborator

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BRs are not handled correctly in shrinkWrap mode
3 participants