Make {MessageList,EmojiReaction}Theme.{light,dark} static final fields #1276
Conversation
lib/widgets/content.dart
Outdated
| @@ -38,91 +38,91 @@ import 'text.dart'; | |||
| class ContentTheme extends ThemeExtension<ContentTheme> { | |||
There was a problem hiding this comment.
nit: perhaps it's intentional, but don't we usually use content: for the commit summary of changes like this?
PIG208
added
integration review
chrisbobbe
force-pushed
the
pr-theme-extension-final-fields
branch
from
022119e to
110e8c4
Compare
|
Thanks for the review! Revision pushed, and I see you've marked for Greg's review. |
lib/widgets/content.dart
Outdated
| style: widget.style | ||
| .merge(ContentTheme.of(_context!).textStyleInlineCode) | ||
| .merge(ContentTheme.of(_context!).inlineCode) |
There was a problem hiding this comment.
I'm not sure about this last commit. It feels confusing to me to have some of these be colors and others text styles and not have any identification of which is which — you can't really guess from the names, as it's arbitrary that e.g. pollNames is only a color while inlineCode is a style. (The poll names are text just as much as the inline code is.)
It's true that the types are distinct and so the type-checker will prevent any real disasters. But it's useful to be able to read and understand the code without having to go track down (or consult the IDE about) the types of things all the time.
The other theme classes don't have this issue because they're entirely colors.
There was a problem hiding this comment.
Oh, yeah I don't mind dropping the last commit :)
chrisbobbe
force-pushed
the
pr-theme-extension-final-fields
branch
from
110e8c4 to
34b31da
Compare
|
Thanks for the review! Revision pushed, atop #1290 for the sake of CI. |
Followup to #1236 (comment):
We can, for
MessageListThemeandEmojiReactionTheme, but not forContentThemebecause the light and dark variants need aBuildContext.