ui [nfc]: Comment with TODO(#95) where we need dark-theme colors #689
Conversation
chrisbobbe
added
integration review
There was a problem hiding this comment.
Cool, looks good — one nit below, and a side comment.
This probably makes a good point to pause on #95 for a bit (after merging the several PRs from Friday/today that are close to merge), and pick up #455 instead. Vlad is working on mobile designs right now, so there'll probably be more to see within a week or two.
When you do pick #95 back up, if some of these colors still don't have a spec at hand, I think that doesn't need to block having a PR that we can merge.
-
A first PR introducing dark mode could leave it in a state where it's never actually activated unless you edit a line of code somewhere to turn it on. That way, it's fine if there are some places that still lack appropriate colors completely and are using light-mode colors. (Preferably with
TODO(#95)comments.)That PR would let us work out how the code and the data flow are organized, even though some of the color values aren't right.
-
Later, for a PR we can merge that actually enables dark mode, we could make placeholder choices by just swapping white for black, and in general lightness L for lightness 1-L. Those should be good enough that the dark mode doesn't look outright busted, even if in some places it doesn't look its best. We can then file a followup issue for pinning down those remaining dark-mode colors.
| @@ -245,6 +245,7 @@ abstract class _HeaderItem extends StatelessWidget { | |||
| @override | |||
| Widget build(BuildContext context) { | |||
| return Material( | |||
| // TODO(#95) need dark-theme color | |||
There was a problem hiding this comment.
nit in commit message:
ui [nfc]: Comment with TODO(#95) where we need dark-theme colors
Related: #95
In these places, I was unable to find a specific dark-theme color by
consulting web or the Figma (as it is now).
Any header-style "Foo: bar" lines should be their own stanza — so add a blank line between "Related: #95" and the paragraph that follows.
There was a problem hiding this comment.
Oh weird; I think maybe it got this way because I added the "Related" line hastily and I forgot there was a commit-message body. There was a commit-message body, but I guess I accidentally lumped it in with the other text in the editor that is not commit-message body:
ui [nfc]: Comment with TODO(#95) where we need dark-theme colors
Related: #95
In these places, I was unable to find a specific dark-theme color by
consulting web or the Figma (as it is now).
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date: Sun May 19 15:44:50 2024 -0700
#
# interactive rebase in progress; [etc.]
There was a problem hiding this comment.
I usually put "Foo: bar"-style lines at the ends of commit messages; do you think we should start putting them at the top of the body?
lib/widgets/code_block.dart
Outdated
| @@ -5,6 +5,8 @@ import '../model/code_block.dart'; | |||
| // Highlighted code block styles adapted from: | |||
| // https://github.com/zulip/zulip/blob/213387249e7ba7772084411b22d8cef64b135dd0/web/styles/pygments.css | |||
|
|
|||
| // TODO(#95) update light-theme colors to match web, then follow web for dark | |||
There was a problem hiding this comment.
Hmm interesting! How do they differ from web today? /cc @rajveermalviya
Oh I guess at some point recently we changed what set of colors we used, and maybe that was after we implemented this feature here. … Yep, that was in 2023-12, definitely more recent.
I think the discussion there kind of petered out, with some open feedback on the new colors and no firm conclusion on what we'd do next. So I would not take this as a blocker for #95.
chrisbobbe
force-pushed
the
pr-more-dark-theme-color-todos
branch
from
1330055
to
bef8340
Compare
|
Thanks for the review! Revision pushed. In this commit: I made the helper a static field on the class, instead of a variable in the build method. |
|
Thanks! Looks good — merging. |
In these places, I was unable to find a specific dark-theme color by consulting web or the Figma (as it is now). Related: zulip#95
To align with web, which labels this color --color-background-private-message-header. (If there is a slight change to the color we use, it's within a rounding error.)
gnprice
force-pushed
the
pr-more-dark-theme-color-todos
branch
from
bef8340
to
b9e585e
Compare
This PR marks several places where we don't yet have enough direction to make a v1 of dark-theme support. In these places, I was unable to find an appropriate dark-theme color by looking at the web app and the Figma (as it is now).
Once we fill these in, it hopefully won't be too long until we have that v1 of dark theme. 🙂
Then some time after that, we'll hopefully have an updated Figma with a dark-theme variant of all the UI pieces. We'll examine that closely and fix places that aren't following it, including places that were wrong because we were following web. (As Vlad says on CZO, "Colors in web might be different from mobile.")