dark_theme_css: Fix border color for tables in message/chat. #29859

PieterCK · 2024-04-26T15:25:32Z

📑 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:

last updated: 2 May 2024

The new dark mode table is under these new effect(s): opacity: 0.2

Before	After

On different elements:

last updated: 2 May 2024

in draft:

in preview:

inside spoiler:

Markdown table css in light mode is still preserved:

last updated: 2 May 2024

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.

I've highlighted where is the css rule that causes this in one of my comment. In one of my prep commit I've also added a change to make all thead background color consistent at all place. Let me know if this isn't needed or should be moved into a different commit.

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

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

alya · 2024-04-27T07:15:46Z

That looks better to me! @terpimost do you have any feedback on this one?

alya · 2024-04-27T07:16:15Z

@PieterCK To fully prepare the PR for review, you'll need to go through the self-review checklist.

PieterCK · 2024-04-28T03:29:16Z

Ah, thank you for pointing it out. I'll not forget to do that next time

terpimost · 2024-04-28T19:18:52Z

I would decrease transparency of the white borders a bit (try 0.5)

PieterCK · 2024-04-29T04:57:03Z

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:

0.4 Opacity	0.5 Opacity	1 Opacity

I've also included several variation of the table under different effects and inside other element.

if matched with spoilers' css:

spoiler blocks' css= border-color: hsla(0, 0%, 50%)

table with border-color: hsla(0, 0%, 50%)

if rounded:

somehow border-radius effect is not symmetrical

with more data:

`border-color: hsla(0, 0%, 50%)`	0.5 Opacity

I'll push the new change after your next input 👍

PieterCK · 2024-04-29T05:02:12Z

Update: included the new table border color css for the tables in preview message

terpimost · 2024-04-29T14:40:16Z

0.4 is better, but I think we can even do 0.2

karlstolley · 2024-04-29T16:14:06Z

These colors and other values should be expressed as CSS variables; let's not go the route of adding more lines to dark_theme.css.

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 app_variables.css. (Those initial light-mode values being converted to variables will make for an excellent first commit.)

PieterCK · 2024-04-30T03:17:57Z

These colors and other values should be expressed as CSS variables; let's not go the route of adding more lines to dark_theme.css.

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 app_variables.css. (Those initial light-mode values being converted to variables will make for an excellent first commit.)

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 app_variables.css. Thanks for leaving a review

PieterCK · 2024-04-30T03:52:40Z

web/styles/dark_theme.css

@@ -792,9 +792,6 @@
    #subscription_overlay .settings-radio-input-parent,
    #settings_page .sidebar-wrapper,
    #settings_page .sidebar-wrapper *,
-    table,


Note: this overrides the new css for rendered markdown table in darkmode.

So far, no regression found

PieterCK · 2024-04-30T03:59:35Z

0.4 is better, but I think we can even do 0.2

Here's some screenshots with the 0.2 opacity!

on different opacity scale:

0.2 Opacity	0.4 Opacity	0.5 Opacity

inside preview:

inside spoiler:

PieterCK · 2024-04-30T04:16:54Z

@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?

css for table header in light theme:

css for table header in dark theme:

karlstolley · 2024-04-30T15:55:18Z

web/styles/app_variables.css

@@ -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%);


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.

alya · 2024-04-30T20:35:48Z

Here's some screenshots with the 0.2 opacity!

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.

alya · 2024-04-30T20:36:03Z

You can just post a comment to note that the description has been updated.

PieterCK · 2024-05-01T03:18:04Z

You can just post a comment to note that the description has been updated.

Ahh, gotcha. I agree, this seems cleaner. Will do! 😃

karlstolley · 2024-05-01T19:02:48Z

@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?

@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.

PieterCK · 2024-05-02T15:47:45Z

web/styles/dark_theme.css

@@ -736,8 +735,7 @@
        background-color: hsl(228deg 11% 17%);
    }

-    & thead,


The markdown table in preview uses this css: background-color: hsl(0deg 0% 0% / 20%); instead if the usual background-color: hsl(0deg 0% 0% / 50%); used else where.

continuation of the highlighted code:

PieterCK · 2024-05-02T15:52:34Z

Update: Added new prep commit to refactor thead background color and called a potential issue/ bug. All details are in the updated PR desc.

alya · 2024-05-02T22:13:19Z

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.

karlstolley · 2024-05-03T21:05:12Z

web/styles/app_variables.css

@@ -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 */


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.

karlstolley · 2024-05-03T21:07:09Z

web/styles/app_variables.css

@@ -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) */


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.

karlstolley · 2024-05-03T21:08:42Z

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.

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.

PieterCK · 2024-05-04T12:03:04Z

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.

Update: Reorganized the new css variables in app_variable.css.

Noted, I'll keep and eye out for regression related to this change.

zulipbot added the size: S label Apr 26, 2024

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch 2 times, most recently from be27289 to da2d8c6 Compare April 28, 2024 04:22

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch from da2d8c6 to cbcba11 Compare April 29, 2024 05:00

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch 3 times, most recently from ceb9ed4 to 70106fd Compare April 30, 2024 03:50

PieterCK commented Apr 30, 2024

View reviewed changes

PieterCK mentioned this pull request Apr 30, 2024

integrations: Update Newrelic integrations. #29730

Open

12 tasks

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch 2 times, most recently from 32112a6 to 9649d19 Compare April 30, 2024 12:22

karlstolley reviewed Apr 30, 2024

View reviewed changes

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch from 9649d19 to 87051a9 Compare April 30, 2024 17:15

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch 2 times, most recently from 683350c to 050d13c Compare May 2, 2024 15:29

PieterCK commented May 2, 2024

View reviewed changes

alya added the maintainer review PR is ready for review by Zulip maintainers. label May 2, 2024

karlstolley reviewed May 3, 2024

View reviewed changes

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch from 050d13c to 1a9fdbc Compare May 4, 2024 11:52

PieterCK added 3 commits May 4, 2024 18:54

css: Refactor css for markdown table border color.

79027be

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.

css: Refactor css for markdown thead background color.

8f27321

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.

css: Add new css for rendered markdown table in dark mode.

596f50d

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.

PieterCK force-pushed the issue-29856-fix-table-border-color-in-dark-mode branch from 1a9fdbc to 596f50d Compare May 4, 2024 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dark_theme_css: Fix border color for tables in message/chat. #29859

dark_theme_css: Fix border color for tables in message/chat. #29859

PieterCK commented Apr 26, 2024 •

edited

alya commented Apr 27, 2024

alya commented Apr 27, 2024

PieterCK commented Apr 28, 2024

terpimost commented Apr 28, 2024

PieterCK commented Apr 29, 2024 •

edited

PieterCK commented Apr 29, 2024

terpimost commented Apr 29, 2024

karlstolley commented Apr 29, 2024 •

edited

PieterCK commented Apr 30, 2024 •

edited

PieterCK Apr 30, 2024

PieterCK Apr 30, 2024

PieterCK commented Apr 30, 2024

PieterCK commented Apr 30, 2024 •

edited

karlstolley Apr 30, 2024

alya commented Apr 30, 2024

alya commented Apr 30, 2024

PieterCK commented May 1, 2024

karlstolley commented May 1, 2024

PieterCK May 2, 2024

PieterCK commented May 2, 2024 •

edited

alya commented May 2, 2024

karlstolley May 3, 2024

karlstolley May 3, 2024

karlstolley commented May 3, 2024

PieterCK commented May 4, 2024

dark_theme_css: Fix border color for tables in message/chat. #29859

Are you sure you want to change the base?

dark_theme_css: Fix border color for tables in message/chat. #29859

Conversation

PieterCK commented Apr 26, 2024 • edited

📑 Overview

On different elements:

Markdown table css in light mode is still preserved:

🧑‍🏭 Concerns & Issue Encountered

The thead background color for markdown table in preview are different from the ones rendered in channel or in draft menu.

alya commented Apr 27, 2024

alya commented Apr 27, 2024

PieterCK commented Apr 28, 2024

terpimost commented Apr 28, 2024

PieterCK commented Apr 29, 2024 • edited

PieterCK commented Apr 29, 2024

terpimost commented Apr 29, 2024

karlstolley commented Apr 29, 2024 • edited

PieterCK commented Apr 30, 2024 • edited

PieterCK Apr 30, 2024

Choose a reason for hiding this comment

PieterCK Apr 30, 2024

Choose a reason for hiding this comment

PieterCK commented Apr 30, 2024

PieterCK commented Apr 30, 2024 • edited

karlstolley Apr 30, 2024

Choose a reason for hiding this comment

alya commented Apr 30, 2024

alya commented Apr 30, 2024

PieterCK commented May 1, 2024

karlstolley commented May 1, 2024

PieterCK May 2, 2024

Choose a reason for hiding this comment

PieterCK commented May 2, 2024 • edited

alya commented May 2, 2024

karlstolley May 3, 2024

Choose a reason for hiding this comment

karlstolley May 3, 2024

Choose a reason for hiding this comment

karlstolley commented May 3, 2024

PieterCK commented May 4, 2024

PieterCK commented Apr 26, 2024 •

edited

PieterCK commented Apr 29, 2024 •

edited

karlstolley commented Apr 29, 2024 •

edited

PieterCK commented Apr 30, 2024 •

edited

PieterCK commented Apr 30, 2024 •

edited

PieterCK commented May 2, 2024 •

edited