Skip to content

Conversation

@stanio
Copy link

@stanio stanio commented Dec 7, 2022

Should sufficiently solve #63:

The ‘color-scheme’ property allows an element to indicate which color schemes it is designed to be rendered with. These values are negotiated with the user’s preferences, resulting in a used color scheme that affects things such as the default colors of form controls and scrollbars as well as the used values of the CSS system colors.

@stanio
Copy link
Author

stanio commented Dec 7, 2022

The style sheet already uses CSS system colors. The color-scheme: light dark declaration just adjusts their actual values to the browser's theme setting.

@stanio
Copy link
Author

stanio commented Feb 6, 2023

Here's a demo:

Dark Light
dark light

@stanio
Copy link
Author

stanio commented Jun 10, 2023

@Rob--W, could you spin a new version using the suggested change? (if you're o.k. with the change)

@jtotht
Copy link

jtotht commented Jul 23, 2024

The style sheet already uses CSS system colors.

Mostly. As it can be seen on the screenshot, there are two non-system colors, which are hard to read in dark mode:

  • .options-link-wrapper a { color: blue; } – for this, not setting a color and deferring it to the browser seems to be enough (links are blue by default). If you want to stay extra safe (avoid visited link color, active link color etc. coming into play), you can explicitly set color: LinkText.
  • #url-protocol.https { color: green; } – this one is even less readable, and I haven’t found any green system colors, so I fear you have to specify a dark mode-friendly color manually.

By the way, thanks for working on this! I don’t have the right to merge and release this (I’m not even a contributor until #78 is accepted), but I’d also love to see this released.

@stanio stanio force-pushed the issue-63-dark-theme branch from ec33672 to a1ff730 Compare July 28, 2024 09:17

#url-protocol.https {
color: green;
color: light-dark(green, ActiveText);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you find another system-color more suitable, f.e. AccentColor or Highlight, or should it be just a brighter green like lime?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should remain a shade of green: https is highlighted because it’s good (secure), and good things are conventionally green, not blue (and most definitely not red like ActiveText). lime looks good.

Copy link

@jtotht jtotht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing if the settings link works as expected (it does), I realized that the settings page is also light-only. I think this PR is okay even if the settings page remains light-only (it’s rarely used anyway), but if you feel like it, you may give that one as well a go.


#url-protocol.https {
color: green;
color: light-dark(green, ActiveText);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should remain a shade of green: https is highlighted because it’s good (secure), and good things are conventionally green, not blue (and most definitely not red like ActiveText). lime looks good.

@stanio stanio force-pushed the issue-63-dark-theme branch from a5ca666 to 33d6f38 Compare July 28, 2024 12:24
}

html, body {
color-scheme: light dark;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put this under :root for consistency? Or even meta color-scheme.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't been aware of the <meta name="color-scheme">, previously. Do you prefer it declared with a <meta> element? I'll replace it accordingly.

@stanio stanio force-pushed the issue-63-dark-theme branch from ea8cba1 to 7433364 Compare July 28, 2024 21:41
@stanio stanio requested a review from Rob--W July 30, 2024 19:10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding <meta name="color-scheme" content="light dark"> here as well. However, there are still a few hardcoded colors (three border colors and a shadow color), which, although aren’t unreadable (there’s nothing to read there), don’t look great.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, there are still a few hardcoded colors (three border colors and a shadow color), which... don’t look great.

Few images for comparison:

dark-prev dark-new
dark-glow light

I don't think anything but a dark (black) shadow makes sense. If both of you and @Rob--W think a glow vs. shadow with the dark scheme is better, I'll update the change appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants