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

Add support for ex units #618

Merged
merged 16 commits into from Dec 8, 2021
Merged

Add support for ex units #618

merged 16 commits into from Dec 8, 2021

Conversation

bselwe
Copy link
Contributor

@bselwe bselwe commented Nov 19, 2021

Adds support for font-relative ex units.

  • Modified parseDoubleWithUnits to parse ex units along with px and em.
  • Added xHeight to SvgTheme (defaults to the font size divided by 2).
  • SvgPicture uses the provided x-height to calculate ex units in SVG elements. If no x-height is provided through the widget's property, then the value fallbacks to (consecutively) DefaultSvgTheme.xHeight or the font size divided by 2.
  • The following SVG elements/attributes support ex units at the moment:
    • svg (width, height)
    • use (x, y)
    • text (x, y)
    • radialGradient (cx, cy, r, fx, fy)
    • linearGradient (x1, y1, x2, y2)
    • image (x, y, width, height)
    • circle (cx, cy, r)
    • rect (x, y, width, height, rx, ry)
    • ellipse (cx, cy, rx, ry)
    • line (x1, y1, x2, y2)
    • stroke-dasharray
    • stroke-dashoffset
    • stroke-width

@bselwe
Copy link
Contributor Author

bselwe commented Nov 19, 2021

@dnfield

@override
bool operator ==(dynamic other) {
if (other.runtimeType != runtimeType) {
return false;
if (identical(this, other)) {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for this change?

This class shouldn't really be expensive enough to compare yet to benefit from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method should not be expensive as it would only check whether two references are the same, right? The reason why I wanted to avoid runtimeType is that I found runtimeType.toString may have different values under minification and saw some recommendations to avoid it (see here). Let me know if you prefer to keep runtimeType here.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, runtimeType toString should be avoided but this does not use that. Please change it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, reverted.

});
});

group(
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we deleting these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because the decoder is now rebuilt only when PictureProvider's theme changes (before PictureProvider had multiple properties like fontSize and currentColor, now it's only a theme that wraps all those properties). Because of that we only need to check if the decoder rebuilds when the theme changes if that makes sense.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh ok

Copy link
Owner

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Overall seems fine, a couple small questions

@bselwe bselwe changed the title Add support for ex support Add support for ex units Nov 24, 2021
@bselwe
Copy link
Contributor Author

bselwe commented Dec 8, 2021

@dnfield I think all your comments should be resolved now. Do you think we can merge this?

@dnfield dnfield merged commit dc9e644 into dnfield:master Dec 8, 2021
@dnfield
Copy link
Owner

dnfield commented Dec 8, 2021

Thanks!

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.

None yet

2 participants