Fix handling of null body2 text style for chip and slider #17311
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gspencergoog
force-pushed
the
chip_body2
branch
3 times, most recently
from
afc10a1 to
51c644f
Compare
HansMuller
approved these changes
May 9, 2018
Contributor
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Just a few requests to improve the docs
Contributor
There was a problem hiding this comment.
Rather than reusing the theme var, maybe make it final and
return new Theme(
child: child,
data: theme.copyWith(
...
Contributor
Author
There was a problem hiding this comment.
Good idea. Done.
Contributor
There was a problem hiding this comment.
I think it would help to qualify "incomplete", like: is incomplete because it only specifies one text style. Because it is incomplete, just replacing ...
Contributor
There was a problem hiding this comment.
This statement isn't as helpful as it should be. Since we have both copyWith and merge, we should provide some rule of thumb here. The see also comment appears to be headed in the right direction...
Contributor
Author
There was a problem hiding this comment.
OK, I reworked those both into descriptive See Also entries.
Contributor
There was a problem hiding this comment.
Same comment as the one about "Using [merge]"
srawlins
added a commit
to srawlins/flutter
that referenced
this pull request
May 11, 2018
* master: Add a golden image test for centered text (flutter#17517) Gallery a11y fix: give the categories and demos pages "route" scope (flutter#17516) Stop Gallery the logo to back-button cross fade shaking (flutter#17513) Correct a typo in InputDecorator, affects computeMinIntrinsicHeight() (flutter#17512) don't fail a test when there are issues deleting a temp dir (flutter#17498) Roll engine to 9ae10ef (flutter#17503) Roll engine to b856303 (flutter#17499) Rebase after package:isolate fixes. (flutter#17289) Add more unit tests for AOT snapshotting (flutter#17493) Update a TODO with issue number (flutter#17494) Fix backdrop demo margin for iPhone X (flutter#17480) Post libtxt/post iOS 11 fidelity fine tuning (flutter#17366) Add onChangeStart and onChangeEnd to slider. (flutter#17298) Use deprecated I/O constants (flutter#17491) Augment `flutter screenshot` with all supported screenshot types (flutter#17478) Roll engine to fade83c (flutter#17488) Fix handling of null body2 text style for chip and slider (flutter#17311) Roll engine to 37e20af (flutter#17481) Mark test as flaky (flutter#17486)
DaveShuckerow
pushed a commit
to DaveShuckerow/flutter
that referenced
this pull request
May 14, 2018
) Before this change, if you specified a non-null textTheme, but the theme you specified didn't have a body2 defined, then creating a ChipTheme would assert (which means creating a ThemeData would fail). This adds handling for this corner case to default to reasonable values in that case. The slider had the same problem, but for accentTextTheme, so I fixed that too. While I had the patient open, Hans and I noticed that TextTheme.merge wasn't doing the right thing in the case where some members were null either, so I fixed that, and added some examples, since merge/copyWith are common operations that are not always well understood. Fixes flutter#17251
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, if you specified a non-null
textTheme, but the theme you specified didn't have abody2defined, then creating aChipThemewould assert (which means creating aThemeDatawould fail).This adds handling for this corner case to default to reasonable values in that case. The slider had the same problem, but for
accentTextTheme, so I fixed that too.While I had the patient open, Hans and I noticed that TextTheme.merge wasn't doing the right thing in the case where some members were null either, so I fixed that, and added some examples, since merge/copyWith are common operations that are not always well understood.
Fixes #17251