msglist: Add channel-feed action from topic narrows #1072
Conversation
This file contains 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
This confused me for a bit just now about the structure of this code.
We already specify this in AppBarTheme.actionsIconTheme.color in our global theme.
The most conspicuous change this makes is to the "back" icon in an AppBar, aka AppBar.leading. Those have been a white or black color from Material defaults; now they get our "icon" color. This matches what's in Figma; and also matches the action icons, for any app bars that have both. Currently our only app bar with an action is also the only one that lacks a "leading": it's on ChooseAccountPage, which is at the root of navigation so there's nowhere for a "back" icon to lead to. But when we add actions in other pages, the mismatch would become conspicuous. This change also turns a handful of less-visible icon buttons from white/black to the icon color: the lightbox's "copy link" or play/pause buttons, and the show/hide-password button on the username/password login page. I think those changes are desirable too. All our other icon buttons that I can find already (either in the source, or by browsing around the app) have a color specified, and aren't affected by this change.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
(Oh, I should upload screenshots for the visual changes — I already went to the trouble of taking them. Short on time this morning but I'll try to do so later today.) |
|
The code LGTM and so does the UI (including those changed icon colors); please merge at will, with or without posting those screenshots. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Also apply our "icon" color to icon buttons more broadly.
Fixes: #1040
Selected commit messages
theme: Apply icon color through IconButtonTheme
The most conspicuous change this makes is to the "back" icon
in an AppBar, aka AppBar.leading. Those have been a white or
black color from Material defaults; now they get our "icon" color.
This matches what's in Figma; and also matches the action icons,
for any app bars that have both. Currently our only app bar with
an action is also the only one that lacks a "leading": it's on
ChooseAccountPage, which is at the root of navigation so there's
nowhere for a "back" icon to lead to. But when we add actions in
other pages, the mismatch would become conspicuous.
This change also turns a handful of less-visible icon buttons
from white/black to the icon color: the lightbox's "copy link"
or play/pause buttons, and the show/hide-password button on the
username/password login page. I think those changes are desirable
too. All our other icon buttons that I can find already (either
in the source, or by browsing around the app) have a color specified,
and aren't affected by this change.
msglist: Add channel-feed action from topic narrows
Fixes: #1040