Skip to content
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

Invert brightness of SVGs in dark mode #33

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Conversation

huyenngn
Copy link
Member

No description provided.

Copy link
Member

@ewuerger ewuerger left a comment

Choose a reason for hiding this comment

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

LGTM

frontend/src/App.css Outdated Show resolved Hide resolved
frontend/src/App.css Outdated Show resolved Hide resolved
frontend/src/components/SVGDisplay.jsx Outdated Show resolved Hide resolved
@Wuestengecko Wuestengecko changed the title fix: Invert SVGs in dark mode Invert brightness of SVGs in dark mode Sep 24, 2024
Copy link
Member

@Wuestengecko Wuestengecko left a comment

Choose a reason for hiding this comment

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

Some bits aren't properly styled in dark mode (although parts of it also happen on master, but I think this PR is a good place to address those):

  • The icons in the top bar next to the light/dark mode switcher (home and cog wheel) stay black, which makes them hard to decipher in dark mode.
  • In the SVG lightbox, the icons at the top do get inverted, but the top bar doesn't, which means it uses black icons on a black bar (where the black bar is nearly invisible compared to the dimmed background as well).
  • Brightness-inverted SVGs with (originally) white background now use a pitch-black background. With the current "dark mode" scheme, where the cards are brighter than the page background, this looks somewhat odd and out of place (especially so on the main page around the model complexity badge, which is much brighter than the instance pages). However, I'm not entirely sure what to do about it - maybe it's fine if we just keep it this way.

Comment on lines 49 to 50
filter: invert() invert(19%) sepia(9%) saturate(1857%) hue-rotate(180deg)
brightness(99%) contrast(87%);
Copy link
Member

Choose a reason for hiding this comment

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

What a filter chain. First of all:

-  filter: invert() invert(19%) sepia(9%) saturate(1857%) hue-rotate(180deg)
-    brightness(99%) contrast(87%);
+  filter: invert(81%) sepia(9%) saturate(1857%) hue-rotate(180deg) contrast(87%);

Second, the heavy over-saturation of 18.57 really messes with the colors, which makes it hard to see e.g. which box is marked with the bold outline for context diagrams. At this point maybe we should also rethink the general idea, and instead of inverting the brightness values, only add a simple filter like this to tone it down a bit:

-  filter: invert() invert(19%) sepia(9%) saturate(1857%) hue-rotate(180deg)
-    brightness(99%) contrast(87%);
+  filter: contrast(1.2) brightness(.8);

What do you think about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just toning it down still leaves it white. Especially when you click to enlarge the SVG, there will be an awful lot of white on your screen for a dark mode.

@huyenngn huyenngn force-pushed the fix-dark-mode-svg branch 3 times, most recently from 2301307 to 841af03 Compare October 1, 2024 12:12
@Wuestengecko Wuestengecko merged commit c04aa19 into master Oct 1, 2024
7 checks passed
@Wuestengecko Wuestengecko deleted the fix-dark-mode-svg branch October 1, 2024 13:56
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