introduce localized text geometry in MaterialLocalizations #11829
Conversation
There was a problem hiding this comment.
nit: space (or newline, typically preferred) after { for named arguments (and matching whitespace on the } side)
There was a problem hiding this comment.
Why is this configurable? This class is the en-US localisation but it's also the interface that other MaterialLocalizations implementations will implement or extend, so it doesn't really make sense for this to be a field.
There was a problem hiding this comment.
Only needed it for testing. But now I'll wait for Hans' PR to go in so I can move this into the DefaultMaterialLocalizations.
There was a problem hiding this comment.
Merged with Hans' code. No changes necessary for typography.
There was a problem hiding this comment.
this should just be hard-coded to the englishLike text theme, since this is the en-US default localisation.
There was a problem hiding this comment.
This will become abstract after Hans' change.
There was a problem hiding this comment.
these should be public since other packages that implement localisations will need to be able to refer to them.
There was a problem hiding this comment.
i believe all the dense ones should have an ideographic baseline.
There was a problem hiding this comment.
I don't think this table should exist. The actual localizations are where this information belongs.
There was a problem hiding this comment.
Should this go in the upcoming DefaultMaterialLocalizations? @HansMuller, WDYT?
There was a problem hiding this comment.
Moved under material/i18n/typography_localizations.dart.
There was a problem hiding this comment.
I continue to this this table shouldn't exist.
There was a problem hiding this comment.
I would probably put the call to MaterialLocalizations.of(context) here too, so that all the inherited-widget logic is in one place.
There was a problem hiding this comment.
how does this differ from the superclass implementation? if it does, please add a comment saying what the difference is
There was a problem hiding this comment.
Done. Also deduped the logic a bit by moving it into a top-level function.
There was a problem hiding this comment.
we should add a TODO about the font fallback issue we discussed
There was a problem hiding this comment.
Past experience has suggested that it's better if the call sites do the defaulting rather than the .of functions. It's ok to return null if there's no _LocalizationsScope in scope (though then you should document that that will happen), but we shouldn't default.
The problem with defaulting is that people don't know the value came from window, so they don't think to set up the onLocaleChanged handler. We saw this a lot with MediaQuery.of defaulting its padding to window.padding if it couldn't find a MediaQuery.
There was a problem hiding this comment.
Rolled back this change. Turns out I don't need it.
There was a problem hiding this comment.
Please document that this returns null if there's no _LocalizationsScope in scope.
yjbanov
left a comment
There was a problem hiding this comment.
D'oh! I forgot to hit the "Submit review" button.
There was a problem hiding this comment.
Only needed it for testing. But now I'll wait for Hans' PR to go in so I can move this into the DefaultMaterialLocalizations.
There was a problem hiding this comment.
This will become abstract after Hans' change.
There was a problem hiding this comment.
Should this go in the upcoming DefaultMaterialLocalizations? @HansMuller, WDYT?
There was a problem hiding this comment.
Done. Also deduped the logic a bit by moving it into a top-level function.
There was a problem hiding this comment.
Rolled back this change. Turns out I don't need it.
|
Merged this with Hans' changes. This is ready for review again. PTAL. |
|
I don't think we should have the table. The kind of typography needed should be a part of the actual localization object, just like whether we're LTR or RTL, what the strings are, etc. That way there's no difference between how we do things and how someone would create a new localization. It's a cleaner separation of concerns and layering. |
WDYT of fcb19a3? |
|
That's much better, but it seems like it'd be even better if the localizations just returned the TextTheme directly. |
Yep, it does. This PR adds |
There was a problem hiding this comment.
Does "The text styles provided by this theme are inherited" mean that localTextGeometry's TextStyles will be inherit:true, or inherit:false? I think we should be more explicit about that.
There was a problem hiding this comment.
are these fontSizes/fontWeights/baselines ever actually used? Can we get away with not specifying them?
There was a problem hiding this comment.
Depends on what you mean by "actually used". Today we support using Typography without the Theme. Example:
testWidgets('geometry defaults are used', (WidgetTester tester) {
final Typography typography = new Typography(platform: TargetPlatform.android);
tester.pumpWidget(new Directionality(
textDirection: TextDirection.ltr,
child: new Text('Hello', style: typography.white.body1),
));
});I'm not sure if our sample apps use it in this way. Probably rarely or not at all.
There was a problem hiding this comment.
Removed fontSizes/fontWeights/baselines in 98612e0. Wasn't a big deal for our tests.
8da8191 to
98612e0
Compare
|
PTAL |
|
Ping? The PR keeps getting outdated as things get added to localizations.dart. I'd like to submit it asap. |
|
|
ba87dc7 to
54fe7ee
Compare
|
This caused a noticeably regression of the stock_build_iteration benchmark. |
|
Looking into it: #12247 |
|
FYI, this broke tests in Should:
|
|
MarkdownStyleSheet.fromTheme() should assert that the theme data is complete. |
What's implemented:
Theme.ofautomatically localizes text properties inThemeDatabased on the nearest availableLocalizationswidgetWhat's not implemented:
ParagraphStyleand not themeable in Flutter yet?)Fixes #11771
/cc @Hixie @HansMuller