content [nfc]: Finish writing down dark-theme color variants for all content #767
Conversation
chrisbobbe
added
a-content
|
(Making this a draft because of that NOMERGE commit.) |
There was a problem hiding this comment.
Great to see! Two nits but otherwise please go ahead and merge (minus the NOMERGE commit).
I scrolled through the feed at tip of branch, and there was a lot that looked wrong but I think it was all outside the content, so expected.
I didn't attempt to verify all the styles taken from web. I'm assuming you did those accurately 🙂. (And if one of them is somehow wrong, it's easy to fix later.)
lib/widgets/content.dart
Outdated
| textStylePlainParagraph: _plainParagraphCommon(context) | ||
| .apply(color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor()) | ||
| .copyWith(debugLabel: 'ContentTheme.textStylePlainParagraph'), |
There was a problem hiding this comment.
Is there something subtle apply is doing here that's different from another argument to copyWith?
There was a problem hiding this comment.
Oh, no, I don't think so. I think I was just thinking of the .copyWith(debugLabel as its own separate step at the end, because we don't want anything to clobber our chosen debugLabel. But this change wouldn't cause it to be clobbered, so sounds good.
lib/widgets/content.dart
Outdated
| colorMessageMediaContainerBackground: | ||
| const HSLColor.fromAHSL(0.03, 0, 0, 1).toColor(), | ||
| colorThematicBreak: | ||
| const HSLColor.fromAHSL(1, 0, 0, .87).toColor().withOpacity(0.2), |
There was a problem hiding this comment.
nit:
| colorMessageMediaContainerBackground: | |
| const HSLColor.fromAHSL(0.03, 0, 0, 1).toColor(), | |
| colorThematicBreak: | |
| const HSLColor.fromAHSL(1, 0, 0, .87).toColor().withOpacity(0.2), | |
| colorMessageMediaContainerBackground: const HSLColor.fromAHSL(0.03, 0, 0, 1).toColor(), | |
| colorThematicBreak: const HSLColor.fromAHSL(1, 0, 0, .87).toColor().withOpacity(0.2), |
to keep it parallel with the light constructor, for comparison.
(It's a little regrettable that the 0.2 opacity gets pushed off past 80 columns that way. But better that than breaking parallelism.)
| // (When GlobalTime appears in a link, it should be blue | ||
| // like the text.) | ||
| color: DefaultTextStyle.of(context).style.color!, |
There was a problem hiding this comment.
Huh interesting. I'm not sure that's entirely intentional! But I reproduce on web, so 🤷
Soon we'll add its counterpart, "dark".
There are still plenty more content styles that we've implemented that need dark-theme variants. We'll add those next, by moving the inline style into `.light` and writing an appropriate variant in `.dark`.
…hemes This doesn't necessarily mean we never want these colors to vary between light and dark. It's just meant to mark them as not needing to be in `ContentTheme`, so it doesn't look like they were excluded by mistake.
There's a lot of duplicated code here; it seems good to be DRY here.
The dark and light values come from the web app.
The dark and light values come from the web app.
The dark and light values come from the web app.
The dark-theme color, like the light-theme one, doesn't have a source in Figma or the web app. I just chose it from some quick testing on my iPhone. (As the comment says, we'll drop the color when we support katex properly, zulip#46.)
See the previous commit for where these colors come from.
This is NFC because _kInlineCodeStyle is just kMonospaceTextStyle
plus a backgroundColor:
final _kInlineCodeStyle = kMonospaceTextStyle
.merge(TextStyle(
backgroundColor: const HSLColor.fromAHSL(0.04, 0, 0, 0).toColor()));
and textStyleInlineMath has its own backgroundColor that was
clobbering that one.
The variable that controls this, --color-markdown-code-background, has had alpha 6%, not 4%, since the redesign color was placed there in zulip/zulip@7817e358f.
The dark and light values come from the web app.
The dark and light values come from the web app.
The dark and light values come from the web app.
The dark and light values come from the web app. This completes the sweep through message-content colors for zulip#95 dark theme. They are all either in ContentTheme with light/dark variants, or still inline but with a comment explaining that they don't need light/dark variants.
chrisbobbe
force-pushed
the
pr-content-theme-finish-sweep
branch
from
2292397 to
fb01562
Compare
|
Thanks for the review!
Done. |
Following #756 (dark-theme variant for code-block syntax highlighting), this completes the sweep through message-content colors for #95 dark theme. By the end of the branch, all colors are defined either in
ContentThemewith light/dark variants, or still inline but with a comment explaining that they don't need light/dark variants.A dev-only commit at the end makes it handy to see the results, but we shouldn't merge it because it just hard-codes dark theme in message content and nowhere else. 🙂 I recommend scrolling through "Combined feed" to see if anything stands out.
There's a tiny color change in light theme—
—as it seems either f452053 was wrong or the web app changed since then. I can make screenshots if that would be helpful.