Skip to content

[Material] Add TabBarTheme #22012

Merged
johnsonmh merged 12 commits intoflutter:masterfrom
johnsonmh:feature-tabBarThemeData
Sep 25, 2018
Merged

[Material] Add TabBarTheme #22012
johnsonmh merged 12 commits intoflutter:masterfrom
johnsonmh:feature-tabBarThemeData

Conversation

@johnsonmh
Copy link
Copy Markdown
Contributor

Feature - This will allow us to create/use custom themes for TabBars

@johnsonmh
Copy link
Copy Markdown
Contributor Author

Note: goldens added in flutter/goldens@26b603d, although I still have failing tests around goldens so I don't think I did it quite right.

Copy link
Copy Markdown
Contributor

@willlarche willlarche left a comment

Choose a reason for hiding this comment

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

So good!!!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is RenderParagraph as high as you can go? Text or RichText don't work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Text doesn't work because it doesn't have a color property, at one point I got it working with RichText, but jacob made an interesting point, that that the RenderObject is a better "source of truth" for something like this, since what we care about is what color gets painted, not as much what the intermediate style properties are. WDYT about that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense! Run by Hans.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using the RenderParagraph object here makes sense.

You can get it like this:

final RenderParagaph renderObject = tester.renderObject<RenderParagraph>(find.text(_tab1Text));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also: check the icon color

@johnsonmh
Copy link
Copy Markdown
Contributor Author

Committed changes addressing comments: @willlarche PTAL

@willlarche
Copy link
Copy Markdown
Contributor

LGTM. Please have Hans review and resolve tests.

@johnsonmh
Copy link
Copy Markdown
Contributor Author

Tests are passing and Will's feedback is incorporated. @HansMuller PTAL!

Copy link
Copy Markdown
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This looks pretty good. Feedback is mostly small-stuff .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also: check the icon color

@johnsonmh
Copy link
Copy Markdown
Contributor Author

@HansMuller I have incorporated the changes you suggested, PTAL when you get a chance!

Copy link
Copy Markdown
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM.

The last couple of comments are just some random formatting suggestions, no biggie.

Really replacing the 40 other occurrences of the lerp doc boilerplate would be a good thing to do for a quick follow-on PR.

@johnsonmh johnsonmh force-pushed the feature-tabBarThemeData branch from a8e2446 to 67e4a2a Compare September 25, 2018 19:49
@johnsonmh johnsonmh merged commit f37c235 into flutter:master Sep 25, 2018
@johnsonmh johnsonmh deleted the feature-tabBarThemeData branch September 25, 2018 21:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants