New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dark_theme_css: Fix border color for tables in message/chat. #29859
base: main
Are you sure you want to change the base?
dark_theme_css: Fix border color for tables in message/chat. #29859
Conversation
That looks better to me! @terpimost do you have any feedback on this one? |
@PieterCK To fully prepare the PR for review, you'll need to go through the self-review checklist. |
Ah, thank you for pointing it out. I'll not forget to do that next time |
be27289
to
da2d8c6
Compare
I would decrease transparency of the white borders a bit (try 0.5) |
Hello @terpimost, thanks for reviewing this PR. Here's what the table looks like on different scale of opacity including 0.5. tables on different opacity scale:
I've also included several variation of the table under different effects and inside other element. if matched with spoilers' css:spoiler blocks' css= I'll push the new change after your next input π |
da2d8c6
to
cbcba11
Compare
0.4 is better, but I think we can even do 0.2 |
These colors and other values should be expressed as CSS variables; let's not go the route of adding more lines to To do this, you'll likely need to make variables out of the comparable light-mode values, and express them and their dark-mode counterparts in |
Oh I see, using only one variable name in the same file makes it easier to manage. I'll make a separate commit for the light mode and the new table border variable in |
ceb9ed4
to
70106fd
Compare
@@ -792,9 +792,6 @@ | |||
#subscription_overlay .settings-radio-input-parent, | |||
#settings_page .sidebar-wrapper, | |||
#settings_page .sidebar-wrapper *, | |||
table, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this overrides the new css for rendered markdown table in darkmode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, no regression found
@karlstolley If that's the case for the table border color, shouldn't the other parts of the table also be refactored in the same way? The obvious one I noticed is the table header background color. I can include it in this PR or open up a followup issue for a more thorough reporting if such an issue isn't already up, what do you think? |
32112a6
to
9649d19
Compare
web/styles/app_variables.css
Outdated
@@ -489,6 +489,9 @@ | |||
--color-background-emoji-picker-emoji-reacted: hsl(195deg 50% 95%); | |||
--color-border-emoji-picker-emoji-reacted: hsl(195deg 52% 79%); | |||
--color-background-emoji-picker-emoji-reacted-focus: hsl(195deg 55% 88%); | |||
|
|||
/* Rendered markdown table color */ | |||
--border-rendered-markdown-tr-th-td: 1px solid hsl(0deg 0% 80%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking better. Let's drop this down just to a color declaration, and follow the pattern of something like --color-border-rendered-markdown-table
. You can then just reference that variable in the color syntax of the border
property--rather than sandwiching all of those values into a single variable.
9649d19
to
87051a9
Compare
This is super helpful! It's best to update screenshots directly in the PR description, so that it's easy for reviewers to see what's currently implemented in the PR. |
You can just post a comment to note that the description has been updated. |
Ahh, gotcha. I agree, this seems cleaner. Will do! π |
@PieterCK sure. Those kinds of fixes would be welcome as you're working in this area, yes. And this PR is still of a manageable size that I think you could do them without introducing a lot of churn here. I'd probably introduce them as prep commits to this PR before the border changes here, since presumably you would only be making variables out of the existing colors, not changing the colors. And make sure you update the PR description with both light- and dark-mode screenshots. |
683350c
to
050d13c
Compare
@@ -736,8 +735,7 @@ | |||
background-color: hsl(228deg 11% 17%); | |||
} | |||
|
|||
& thead, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Added new prep commit to refactor thead background color and called a potential issue/ bug. All details are in the updated PR desc. |
The screenshots all look reasonable to me, thanks! Marking for maintainer review, since the next step is for @karlstolley to a look at the latest set of changes. |
web/styles/app_variables.css
Outdated
@@ -491,6 +491,9 @@ | |||
--color-background-emoji-picker-emoji-reacted: hsl(195deg 50% 95%); | |||
--color-border-emoji-picker-emoji-reacted: hsl(195deg 52% 79%); | |||
--color-background-emoji-picker-emoji-reacted-focus: hsl(195deg 55% 88%); | |||
|
|||
/* Rendered markdown table color */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits here:
- Let's put these just above the
/* Markdown code colors */
portion of the file - Make the heading something more generic, like
/* Markdown colors */
, so that additional variables can be set up there in the future.
web/styles/app_variables.css
Outdated
@@ -792,6 +796,10 @@ | |||
--color-border-emoji-picker-emoji-reacted: hsl(0deg 0% 0% / 90%); | |||
--color-background-emoji-picker-emoji-reacted-focus: hsl(0deg 0% 20% / 70%); | |||
--color-border-add-subscription-button-focus: hsl(0deg 0% 67%); | |||
|
|||
/* Rendered markdown table color (dark) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again with the reorganization. Be sure you order the new variables the same as in the light area of the file, too. These are flipped.
A few code-organization nits. I'm not certain we won't see some regressions from striking those table-oriented element selectors in dark mode, but the fixes for those would be to use more specific selectors anyway. And because it's a border color, any regressions that might be discovered won't affect functionality. |
050d13c
to
1a9fdbc
Compare
This refactor aims to make managing css for rendered markdown table easier by abstracting the css for table border color into a new variable in app_variable.
This refactor aims to make managing css for rendered markdown table easier by abstracting the css for thead background color into a new variable in app_variable.
Add a new dark mode css in app_variable.css for rendered markdown tables. This rule sets a brighter color for tables in messages / chat to make it more visible when the user is using dark theme. Fixes zulip#29856.
1a9fdbc
to
596f50d
Compare
Update: Reorganized the new css variables in Noted, I'll keep and eye out for regression related to this change. |
π Overview
Currently, there's a block of css rule that sets a group of elements to have a dark border color in
dark_theme.css
:This PR adds a new rule that overrides that specific css rule. This new rule specifically targets tables inside message/ chat and set a brighter color border for them.
Fixes #29856
CZO
Screenshots and screen captures:
The new dark mode table is under these new effect(s):
opacity: 0.2
On different elements:
in draft:
in preview:
inside spoiler:
Markdown table css in light mode is still preserved:
in channel, sent:
in draft:
in preview:
π§βπ Concerns & Issue Encountered
The thead background color for markdown table in preview are different from the ones rendered in channel or in draft menu.
different thead background color:
in this PR, all thead background color are the same:
if the background color from the preview table is used instead:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: