- Notifications
You must be signed in to change notification settings - Fork30k
Add aisSystemTextScaler matcher#160120
Conversation
| } | ||
| @override | ||
| double scaleFontSize(double unscaledFontSize) => textScaleFactor * unscaledFontSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a test for this?
| /// | ||
| /// If `withScaleFactor` is specified and non-null, this matcher also asserts | ||
| /// that the [TextScaler]'s' `textScaleFactor` equals `withScaleFactor`. | ||
| Matcher isSystemTextScaler({ double? withScaleFactor }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is a non-system text scaler? Can you add something to the docs that describes the difference between a system text scaler and a non-system one?
And for my own understanding: Why should I care in my tests whether the text scaler is system or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A "system" one is one that directly reflects the user's preference while the developer could introduce a MediaQuery with a "non-system" scaler (with a fixed 1.0 scaling factor, for example). This is useful (I guess) to make sure the text scaler from a certain BuildContext is using the user's preference instead of using some fixed scales.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wanna add that context to the doc? I think it would be useful to specify what cases would fail this check (i.e. when developers introduce their own text scaler in a widget)
goderbauer left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
| /// | ||
| /// If `withScaleFactor` is specified and non-null, this matcher also asserts | ||
| /// that the [TextScaler]'s' `textScaleFactor` equals `withScaleFactor`. | ||
| Matcher isSystemTextScaler({ double? withScaleFactor }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wanna add that context to the doc? I think it would be useful to specify what cases would fail this check (i.e. when developers introduce their own text scaler in a widget)
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visitWriting a golden file test for Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
justinmc commentedFeb 26, 2025
@LongCatIsLooong Heads up on this old PR if you plan to return to it after break. |
autosubmit label was removed for flutter/flutter/160120, because - The status or check suiteLinux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
ac71188Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This is for#159999. That PR breaks registered tests so a new matcher is added for soft transition & making it slightly easier to write tests that verify nothing is shadowing the system text scaler in the widget tree.
If this approach sounds plausible & gets merged, I'm going to:
PlatformDispatcher#159999 as ready for review.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel onDiscord.