Skip to content

msglist: Implement MessageListTheme, with dark-theme colors - #843

Merged
gnprice merged 18 commits into
zulip:mainfrom
chrisbobbe:pr-message-list-theme
Jul 29, 2024
Merged

gnprice merged 18 commits into
zulip:mainfrom
chrisbobbe:pr-message-list-theme

Conversation

@chrisbobbe

Copy link
Copy Markdown
Member

Related: #95

@chrisbobbe chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 27, 2024
@chrisbobbe
chrisbobbe requested a review from gnprice July 27, 2024 01:16
@chrisbobbe

Copy link
Copy Markdown
Member Author
Light Dark
A1AC6674-F80C-4C3D-AC5E-5F2418628297 1180DF79-FFD4-4C3B-9E71-4A324FB3A5CC

Just because web has a dark-theme variant for this color, and the
Figma doesn't yet.

(Web actually expresses this with separate `color` and `opacity`
attributes. We just collapse those into a Color with the right
alpha.)
For the background color, the unread-marker color seems like a very
reasonable starting point, since that's what we're using in light
theme.

White seems reasonable for dark theme, just like in light theme.
There shouldn't be an issue with contrast; I expect the background
will be darker, if anything, in dark theme.

We can always refine these styles later.
The dark-theme variant is copied from the web app.

We'll cover the other items one by one, for ease of review, next.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
The dark-theme variant is copied from the web app.
Web expresses this as separate `color` and `opacity` attributes. We
combine those attributes into a single Color, where the alpha is the
alpha from `color` multiplied by the `opacity`.

So, for light theme: 1 * 0.8 = 0.8
And for dark theme: 0.75 * 0.8 = 0.6
…ad hoc

This should have more reasonable contrast, but we need an approved
color from Vlad.

@gnprice gnprice left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! The code all looks great, and I manually tested a bit and everything looks good.

Merging. This commit disappeared in the rebase:
909633ede msglist [nfc]: Set text color in MarkAsReadWidget, matching default

because it effectively happened as part of 0d577a0 / #710 (I think with some code you supplied in a review).

Comment thread lib/widgets/message_list.dart Outdated
Comment on lines 61 to 62

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For this and any similar spots that come up, I think the strategy will probably be:

  • Pick something reasonable, as you did here.
  • When dark theme is otherwise ready, ship it, and close ui: Implement dark theme #95.
  • File a follow-up issue to track Vlad looking around in the app for anything he'd like to tweak about dark theme, and us applying those changes. In that issue, mention this and any other such items as areas for him to particularly look at.

gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this pull request Sep 9, 2024
After sweeping through the message-list screen to implement dark
theme (zulip#843), in late July, the Flutter framework merged
flutter/flutter#143501, in early August. This made it so our
`foregroundColor` no longer controlled the icon color, as promised
in the dartdocs of `foregroundColor` and `iconColor`.

I opened an issue for Flutter about the inconsistency with the doc:
  flutter/flutter#154644
and sent a PR to resolve it, by updating the doc (which the author
of 143501 had said was the right fix):
  flutter/flutter#154646

Fixes: zulip#926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration review Added by maintainers when PR may be ready for integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants