-
Notifications
You must be signed in to change notification settings - Fork 374
VUFIND-1590: Add dark mode color-scheme #4472
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
base: dev
Are you sure you want to change the base?
VUFIND-1590: Add dark mode color-scheme #4472
Conversation
maccabeelevine
left a comment
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.
| $input-border-color: #888 !default; | ||
| $form-select-border-color: #888 !default; | ||
|
|
||
| $breadcrumb-bg: #f5f5f5 !default; |
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.
As I understand it, we would have to not define any colors using these bootstrap global properties, as these are not specific to any color mode.
| @include color-mode(dark, true) { | ||
| .top-title { | ||
| color: #fff; | ||
| } | ||
| } |
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 would be an example of overriding a locally defined color, in this case for .top-title. It works fine.
It would be more elegant IMHO to put this @include within the main styles for .top-title above. But unfortunately, supporting that kind of non-root-level media query causes problems with some of the built-in Bootstrap styles. Also, since we handle different breakpoints as top-level @includes just above here, I don't see any harm in handling color mode the same way.
| @@ -0,0 +1,25 @@ | |||
| /* Override of built-in color mode, to output both types */ | |||
|
|
|||
| @mixin color-mode($mode: light, $root: false) { | |||
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.
I am overriding the provided color-mode mixin, which supports only one mode at a time -- either system-defined or DOM-defined color-scheme, but not both. So, I'm outputting the CSS for both types.
| // Handle media-query $color-mode-type | ||
| @if $root == true { | ||
| @media (prefers-color-scheme: $mode) { | ||
| :root:not([data-bs-theme]) { |
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 :not is so that the media query (system-defined) color scheme never overrides a DOM-defined color scheme.
|
|
||
| $alert-default-bg: #f5f5f5 !default; | ||
| $alert-default-border-color: darken($alert-default-bg, 7%) !default; | ||
|
|
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.
These styles in variables.scss replace the global (not color-scheme specific) declarations in bootstrap-variable-overrides.scss.
| ?> | ||
| <html lang="<?=$this->layout()->userLang?>"<?php if ($this->layout()->rtl): ?> dir="rtl"<?php endif; ?>> | ||
| <html | ||
| <?php /* data-bs-theme="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.
Uncomment this line (for now) to simulate javascript-specified dark mode. At this initial POC stage I'm more interested in how the code would actually support dark mode than how the user would indicate that they want it.
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.
In the meta tags (better) or in the CSS, I think we should set the color-scheme. This will cause some unspecified colors to follow the system, but it is a very easy way to implement user preference of color-scheme.
<meta name="color-scheme" content="light dark">https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/meta/name/color-scheme
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.
I may be misreading that MDN page, but that meta tag seems to be for the document to say which color schemes it supports (and order of preference), rather than expressing the user's preference?
That said, I'm fine using that meta tag to echo/reflect the user's preference (if they have expressed one via VuFind) -- just implemented that. I don't see any immediate changes but as you say there may be some.
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.
Yes, but this will follow the system. User preference will have to be applied and stored separately - one of the current blind spots of modern web dev. If you change the meta tag to just "dark", it will apply a dark theme. You can then use the prefers-color-scheme and light-dark css tools.
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.
Ok I've made a few more significant additions
- The color scheme selection within VuFind itself can be done via a top menu, once some config is enabled.
- There's protection so only color schemes supported by a given theme can actually be applied, even if it's a system-specified color scheme.
It's ready for a broader look please when folks have time. To be clear there are at least two big aspects to this that need input
- How the user or the system specifies the color mode. Which I've done work on in these latest changes, and the implementation is at least a real suggestion.
- How bootstrap5 deals with a dark mode request across all the scss files, which was the original point of the POC and is still very much at that stage, see various comment threads. There will be a lot more to do before finishing the work on bootstrap5 but I want to get input and eventually consensus on the approach first.
| ; here will be used, or 'light' if there is none specified. | ||
| ; If your local theme supports more than light mode, add it to the array below. | ||
| supported_color_schemes[bootstrap5] = "normal,dark,light" | ||
| supported_color_schemes[sandal5] = "light" |
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.
Any thoughts on whether it's worth building dark-mode compatibility for sandal5? All I can say is it seems it will be a lot more work than bootstrap5 will be, there are color specified all around that theme's scss files.
| ; VuFind will only respect a color scheme choice (whether from VuFind or the system) | ||
| ; if it's supported by the current theme. If not, the first color theme specified | ||
| ; here will be used, or 'light' if there is none specified. | ||
| ; If your local theme supports more than light mode, add it to the array below. |
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.
The effect here is that any local themes will disallow dark mode by default, even though it's globally enabled in bootstrap config. The theme developer would need to actually test its support before adding it to the list below.
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.
Arguably the changes to this file could be a separate PR, if someone prefers or there's a lot to discuss about it (beyond color scheme).
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.
Honestly I don't love adding anything to the top menu bar, but I couldn't think of a better place to put this in the built-in themes. Unless it's a user preference, but I think it should parallel theme by being stored in a cookie and not requiring auth. Still I am open to better places to put this.
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.
I think that the header is the obvious place to look for the toggle, so even though it may feel crowded, it's the right place. I'm not sure how many institutions in real life actually have all the possible options enabled in the header. Perhaps it's not that crowded usually..
|
@maccabeelevine This is a great start! Thanks for taking the initiative. :) Here's a smattering of my initial thoughts...
|
demiankatz
left a comment
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.
Thanks, @maccabeelevine -- didn't have time for a full review but started by taking a look at the ColorSchemeManager.
| * @param string $currentTheme Current theme | ||
| */ | ||
| public function __construct( | ||
| protected Config $config, |
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.
Worth using an array here instead of a Config object to avoid dependencies on Config objects in new code?
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.
Done
|
I've made some changes per @EreMaijala 's suggestions. I also implemented a few more specific dark mode fixes, to test out the pattern more and see if it makes sense.
Agreed and done. My only problem is with "Auto". What I would really like is to use the half-dark/half-light circle as the icon for Auto, and then have icon for the drop-down itself reflect whichever value (dark/light/auto) is actually selected. That's how the Bootstrap document icon works. However it would contradict what we do with the Theme and Language drop-downs where we don't just show the selected value as the menu label. I think that might actually be a good idea, but it would take some UX thought/discussion to make sure it's clear what each menu represents. If folks think that's worth exploring, I can do so as a separate PR.
I need some help understanding this contrast function please. I see it referenced twice in autocomplete.scss but nowhere else, and I see it was introduced in the SASS conversion, but I don't actually see where it's defined. Bootstrap includes a Sass function
Agreed -- I added a to-do above for a follow-up PR.
Yes, this is a concern. I agree with you (and I think @crhallberg ) that CSS variables are the way to go, see my additions to variables.scss to use Bootstrap's CSS variables in place of the SASS variables in bootstrap-variable-overrides.scss. But where Boostrap hasn't created a CSS variable to replace the SASS one for something yet, I'm not really inclined to do so either, and then have to backtrack once they get around to it later ... I'd rather just use VF-specific class selectors. But maybe I need a concrete example where it makes sense to do things differently. Also regarding Bootstrap changes, I do see there's been work done since our current Bootstrap 5.3.3, especially in 5.3.4 from April (the latest is 5.3.7 from June). And then I see there are more PRs in the works. Do we have a plan in general for upstream changes to Bootstrap 5? We could upgrade our bootstrap5 theme to point to 5.3.7, but that would require work by all the local themes that extend it, and so on every time.
The pattern I've been following so far is to make adjustments to the individual components in their own .scss files, as they are already, i.e. buttons.scss or search.scss. I think it does make sense to deal with color properties alongside the others for each component. That said, there are certainly some global color definitions in bootstrap.scss, variables.scss and accessibility.scss that should probably be consolidated somehow. And this may not be the best approach if folks disagree.
Yes -- many, many more styles to go! I want to get consensus on all the issues above before taking it too far in the the wrong direction. |
Apparently Bootstrap 5.4 will expand the use of variables, but the work is still in progress. So one option is to just wait for a while. :) |
Right now, we don't have access to many CSS variables. The bigger problem is that Bootstrap uses most CSS variables in a read-only way (they get baked into CSS when compiled). I found this comment that implies they aren't considering exposing all variables until Bootstrap 6. I do hope your link tells of a decision to do it sooner and more piecemeal! |

This is an initial POC of using Bootstrap 5.3's new built-in support for color modes and specifically
@media (prefers-color-scheme: foo)) and user-specified color scheme (via the DOM)TODO
Follow-up PRs: