[vector_graphics_compiler] support percentage units SVG shape attributes #10577
Conversation
Signed-off-by: Steven Landow <[email protected]>
github-actions
bot
added
p: vector_graphics
triage-engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for percentage units in SVG shape attributes like rect, circle, ellipse, and line, which is a valuable feature. The changes correctly resolve percentage values against the viewport dimensions, and the circle radius calculation correctly uses the normalized diagonal as per the SVG specification. The addition of new tests to verify this functionality is also great. I've found a minor issue with a misleading comment in the new percentage parsing logic and have suggested a correction to improve clarity. Overall, the changes are well-implemented.
jtmcdole
left a comment
jtmcdole
left a comment
There was a problem hiding this comment.
Added a question. Sorry for the delay.
| parserState.attribute('cy', def: '0'), | ||
| percentageRef: vh, | ||
| )!; | ||
| // For radius percentage, use the normalized diagonal per SVG spec. |
There was a problem hiding this comment.
can you link to the spec section?
|
|
||
| // Handle percentage values first. | ||
| // Check inline to avoid circular import with parsers.dart. | ||
| final bool isPercent = rawDouble?.endsWith('%') ?? false; |
There was a problem hiding this comment.
Is the provided rawDouble guaranteed to be trimmed? It seems like it would be safer to trim before doing this check.
| final bool isPercent = rawDouble?.endsWith('%') ?? false; | ||
| if (isPercent) { | ||
| if (percentageRef == null || percentageRef.isInfinite) { | ||
| // If no reference dimension is available, treat as 0. |
There was a problem hiding this comment.
This says "treat as 0", but it returns null, not 0, so it's not clear to me what this comment means.
There was a problem hiding this comment.
I ended up changing it and forgot to update the comment.
| @@ -1,7 +1,11 @@ | |||
| ## NEXT | |||
There was a problem hiding this comment.
See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#updating-a-changelog-that-has-a-next for why CI is failing the changelog check.
There was a problem hiding this comment.
I should probably revert the pubspec version change, right?
3 participants
Adds percentage unit support for SVG shape attributes
SVG shapes like
<rect>,<circle>,<ellipse>, and<line>can have percentage values for their position and size attributes (e.g., width="50%", cx="50%"). Previouslythese weren't handled correctly. Easy to test with any image like https://placeholdit.com/600x400/dddddd/999999
This adds:
List which issues are fixed by this PR. You must list at least one issue.
I'm not sure if it fixes these issues, as they have no description. I can file a new issue if needed.
flutter/flutter#158844
flutter/flutter#158845
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3