content/msglist: Update some elements closer to new, 2023+ design, to prepare for light/dark #657
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
The code block background is currently white anyway, so this has been redundant. But we're about to set it to something off-white, to align with web, and this white background color on the text spans would interfere with that.
chrisbobbe
changed the title
gnprice
reviewed
May 10, 2024
lib/widgets/content.dart
Outdated
Comment on lines
414
to
412
| borderColor: _borderColor, | ||
| // (The other caller, MathBlock, passes a nontransparent color.) | ||
| // TODO(#46) remove comment about MathBlock when TeX is implemented; | ||
| // presumably it won't apply anymore. | ||
| borderColor: Colors.transparent, |
There was a problem hiding this comment.
Does this even need a comment? Presumably we'll remove the borderColor parameter itself when that happens.
lib/widgets/content.dart
Outdated
Comment on lines
828
to
829
| // TODO(#646) when self-user is non-silently mentioned, | ||
| // differ font color between direct and wildcard mentions |
There was a problem hiding this comment.
nit:
Suggested change
| // TODO(#646) when self-user is non-silently mentioned, | |
| // differ font color between direct and wildcard mentions | |
| // TODO(#646) when self-user is non-silently mentioned, | |
| // distinguish font color between direct and wildcard mentions |
At least for me, "differ" is only an intransitive verb.
(Specifically by eliminating the border and using a particular shade of gray for the background.) Also the background of inline code, which in web uses the same CSS variable as the background color for code blocks.
…ound As the TODO comments say, there's more we have to do to bring this up-to-date with the new, 2023+ design. But now at least we've removed a style that doesn't appear at all in that new design, and we're left with something we can make a dark-theme variant for by straightforwardly checking what web does.
With Chrome's web inspector and Mac's "Digital Color Meter", I verified that our color now matches web's color. Since GlobalTime's clock icon is a text-like element, change it to the new color too, and add a content test that makes sure the colors match each other, and match the DefaultTextStyle that content elements expect to inherit from. I didn't find any other text-like elements that need extra treatment like this.
chrisbobbe
force-pushed
the
pr-update-content-and-msglist-colors
branch
from
456a53d to
cedbb08
Compare
|
Thanks! Done. |
This was referenced May 15, 2024
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.
It will be helpful to consult the web app when we define the dark-theme styles of our content elements. This PR prepares for that. I went through all the content elements as we've implemented them, and for all the things that should differ between light and dark theme, ensured that our light-theme style matches the web app.
I also brought the default content text color in line with the web app. (I mention this separately because this color is an attribute of the message list, not content itself, I guess; at least that's how it seems in the code.)
Related: #157
Related: #95