Add a dark theme #8604

prymitive · 2021-03-15T19:45:04Z

This implements dark mode theme using https://github.com/ForEvolve/bootstrap-dark.
User is able to set theme to:
a) light - same as current theme and the default one
b) dark - dark version using bootstrap-dark
c) auto - either light or dark version will be used depending on browser preference, if browser doesn't support media queries for prefers-color-scheme light theme will be used

To make all of this work existing CSS rules were moved into themes folder and converted to scss, both light and dark styles will import that shared CSS file but each might pass different variables to customise generated CSS. Some parts of the shared CSS file use bootstrap and bootstrap-dark variables directly, to avoid redefining what's already defined there.

Signed-off-by: Łukasz Mierzwa l.mierzwa@gmail.com

roidelapluie · 2021-03-15T19:54:52Z

Thanks for this!!

I can't build it it seems:

cd web/ui/react-app && yarn --frozen-lockfile
yarn install v1.22.5
[1/4] Resolving packages...
[2/4] Fetching packages...
info fsevents@2.1.2: The platform "linux" is incompatible with this module.
info "fsevents@2.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
info fsevents@1.2.13: The platform "linux" is incompatible with this module.
info "fsevents@1.2.13" is an optional dependency and failed compatibility check. Excluding it from installation.
info fsevents@2.3.2: The platform "linux" is incompatible with this module.
info "fsevents@2.3.2" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning " > eslint-config-react-app@5.2.1" has unmet peer dependency "babel-eslint@10.x".
[4/4] Building fresh packages...
Done in 41.77s.
>> building React app
building React app
yarn run v1.22.5
$ react-scripts build
Creating an optimized production build...
Failed to compile.

./src/App.tsx
Cannot find file './contexts/ThemeContext' in './src'.


error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

prymitive · 2021-03-15T19:57:16Z

Missed one file when making the PR, should be there now

roidelapluie · 2021-03-15T21:01:12Z

Thanks! This looks good, some elements are still bright and not adapted, but from an end user perspective, I like this.

prymitive · 2021-03-15T21:05:23Z

I’d hope I’ll be able to adopt more elements to bootstrap styles, like query autocomplete, which would improve consistency and reduce amount of custom css needed.

juliusv · 2021-03-15T22:11:22Z

@prymitive Awesome, will take a look!

Don't put any energy into the expression input field yet, I'm going to replace that in the next weeks with the new and awesome PromQL editor functionality from https://github.com/prometheus-community/codemirror-promql and PromLens.

juliusv · 2021-03-15T22:24:14Z

One thing I noticed: switching themes while having a graph open makes the app crash like this:

Seems like the graph container height briefly becomes 0 during the theme switch, and then handleResize() chokes on that.

juliusv · 2021-03-15T22:26:23Z

Also got this error sometimes when trying to graph an expression, not sure if it's related:

prymitive · 2021-03-16T11:17:06Z

Yes, I've noticed some issues with that as well, might need to make some css theme-free or look closer into how height is being calculated.

prymitive · 2021-03-16T17:53:46Z

Moved most of the css back, except for those rules that are templated via scss variables. Hopefully this will prevent element sizes from resetting on theme swap.

juliusv · 2021-03-17T09:45:00Z

Cool! Just some more cursory testing for now:

Still got the Cannot read property 'save' of null error above once, when graphing prometheus_http_request_duration_seconds_count, but I'm not yet sure how to reproduce it again.
The graph time control input fields (range, etc.) are now all the same width, whereas they had custom widths before.

prymitive · 2021-03-17T09:58:44Z

Yeah, I do see failures too, will put this on hold as a draft and will add storybook first so once this is ready we can use storybook to compare all UI elements and see differences between current UI and new themes.

prymitive · 2021-03-20T13:20:26Z

Still got the Cannot read property 'save' of null error above once, when graphing prometheus_http_request_duration_seconds_count, but I'm not yet sure how to reproduce it again.

I think what happens here is that when you toggle themes:

some styles reset
that trigger graph size change
which triggers resize detector
which triggers handleResize()
which calls plot()
which triggers a graph redraw via setTimeout()
if there's another re-render before setTimeout() triggered flot happens it might destroy the canvas and once graph redraw finally runs it gets a null

Or so it seems so. The short term fix is to fix styles so there is no graph resize on theme change.

prymitive · 2021-03-20T21:13:16Z

I think that most of the issues are due to lazy loading theme css.
The effect is that upon first render there is no bootstrap style applied at all, since body now must have either bootstrap or bootstrap-dark class to use bootstrap. Those classes are applied in Theme component that also lazy loads needed css file, so on first render we have body with no class and no styles, we render a bunch of components, in the mean time css loads and all styles are applied, which triggers a bunch of resizes and other updated, and that isn't all handled well.
This is mostly visible on the graph page where:

input gets wrong size - there some js code that set fixed size via style attributes, with lazy loaded css we end up with height of an unstyled input, which is less that what bootstrap will have
flot charts - we see crashes, which seems it's related to some re-renders that are done via setTimeout in the flot lib

So for now I've disabled lazy loading, both themes are loaded with App.css and also added bootstrap class to body, so it uses light theme by default. This should mitigate those issues for now and we'll be able work on making dark theme usable.
We can address lazy loading and graph page issues caused by it later.

prymitive · 2021-03-21T16:28:57Z

Cleaned up the PR a bit and hopefully the UI looks good now.
Lazy loading for theme css would mean 25KB of css instead of 50, but it also introduces an extra step when it all loads and there's a spinner for split seconds, so getting rid of it will make the UX a bit better, so it's not all bad that we can't have it.
Need to add some tests and finish storybook work so we can see all UI elements in various states for both themes easily.

juliusv

Looking pretty great already! :) Just a couple of nits, and of course at least some minor tests would be good to have before merging.

I still got the Cannot read property 'save' of null error once by the way, so I'm not sure whether it's really related to the lazy loading (now that you've removed that).

juliusv · 2021-03-22T10:23:19Z

web/ui/react-app/src/Theme.tsx

+        </Button>
+        <Button
+          color="secondary"
+          title="Use browser preferred theme"


Nit: Either

Suggested change

title="Use browser preferred theme"

title="Use browser's preferred theme"

or (preferred, compound adjective form, http://www.kentlaw.edu/academics/lrw/grinker/LwtaCompound_Adjectives.htm):

Suggested change

title="Use browser preferred theme"

title="Use browser-preferred theme"

juliusv · 2021-03-22T10:33:10Z

web/ui/react-app/src/pages/graph/ExpressionInput.tsx

@@ -215,12 +215,10 @@ class ExpressionInput extends Component<ExpressionInputProps, ExpressionInputSta
                  value={value}
                />
                <InputGroupAddon addonType="append">
-                  <Button className="btn-light border" title="Open metrics explorer" onClick={this.openMetricsExplorer}>
+                  <Button title="Open metrics explorer" onClick={this.openMetricsExplorer}>


Due to the removal of btn-light, this button is now always dark, even in light mode.

This one is a bit tricky since there are no border or other visual separators for buttons in a button group

https://getbootstrap.com/docs/4.6/components/button-group/

Which means that when you have a button group with buttons of different colour it tends to look a bit "raw", at least to my eye. I'll try to add some custom style to it.

juliusv · 2021-03-22T10:44:40Z

web/ui/react-app/src/themes/light.scss

+@import '~bootstrap/scss/variables';
+
+$config-yaml-color: #333;
+$config-yaml-bg: $jumbotron-bg;


Rather than repurposing $jumbotron-bg (whose name is confusing in this context, and whose color is a bit darker than what we used here previously), I think let's use a custom color?

juliusv · 2021-03-22T10:45:58Z

web/ui/react-app/src/themes/shared.scss

+  border-bottom: 1px solid $nav-tabs-border-color;
+}
+
+.rule_cell {


Let's use the opportunity to make the old under_scored CSS class names (rule_cell and alert_cell) consistent with the dash-separated ones :)

juliusv · 2021-03-24T08:39:57Z

Note: This will need a rebase, we just merged the new expression editor yesterday: #8634

I can do the dark themeing for the editor itself in the end (otherwise you have to learn CodeMirror styling specifics), so don't worry about that part.

prymitive · 2021-03-24T10:45:51Z

Rebased and addressed (most?) comments

juliusv · 2021-03-24T10:50:57Z

web/ui/react-app/src/pages/graph/ExpressionInput.tsx

@@ -215,12 +215,10 @@ class ExpressionInput extends Component<ExpressionInputProps, ExpressionInputSta
                  value={value}
                />
                <InputGroupAddon addonType="append">
-                  <Button className="btn-light border" title="Open metrics explorer" onClick={this.openMetricsExplorer}>
+                  <Button className="metrics-explorer" title="Open metrics explorer" onClick={this.openMetricsExplorer}>


Nice, I like how it looks now! The class name is a bit confusing since metrics-explorer is also used for the metrics explorer itself. Maybe metrics-explorer-btn or something like that would be clearer?

I was hoping that being able to target it with button.foo is enough to make it clear but let's rename it for clarity

Yep, I just think the class name itself would have been a bit of a misnomer otherwise... thanks!

juliusv · 2021-03-24T11:02:58Z

@prymitive So now we have to collaborate somehow on bringing the new editor styling into this. I can't push to your private PR branch, but one option would be to open a short-lived feature branch in Prometheus itself where we can both merge stuff into after each other? I'd just like to avoid merging it direclty into main with known brokenness for the new editor.

Or alternatively, if you feel bored (because today I won't have much time at least): Basically what needs to be done is to apply some of the same input group styling changes to the new editor as well, and then update the CodeMirror theme + highlighting colors in https://github.com/prometheus/prometheus/blob/main/web/ui/react-app/src/pages/graph/CMTheme.tsx to add a dark theme. Here's some examples from the base theme in CodeMirror for how to target light and dark theme selectors: https://github.com/codemirror/view/blob/main/src/theme.ts#L71-L72. And then the EditorView.theme() call can take a second parameter of the form {dark: <boolean>} to indicate to CodeMirror whether to style the editor with the dark theme or not.

juliusv · 2021-03-24T11:05:07Z

Btw., the .expression-input .cm-expression-input styling from https://github.com/prometheus/prometheus/pull/8634/files seems to have not made it over into here in the rebase.

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

We can override margins via bootstrap css classes instead of loading custom css module. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

This button had some custom css based on light bootstrap theme so it needs to be adjusted for dark theme. This change re-uses bootstrap styles used for input components instead of copying color values Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

This makes the whole input group look consistent in dark mode as the old styles were made to blend it with the default bootstrap theme. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

This change splits current CME theme into 3: 1 - base theme used for both light and dark mode 2 - light mode specific theme that overrides base 3 - dark mode specific theme that overrides base To make it all work we also need to move theme to dynamic config, so when theme value in ThemeContext changes CME input will apply a new theme. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

bootstrap-dark breaks scrolling on the metrics modal, so we need an extra rule to fix that. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

This completes splitting styles into light and dark theme. It also fixes some small issues with themes as now all styles from App.css are applied correctly. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

prymitive · 2021-04-15T09:31:54Z

Rebased and move App.css to _shared.scss (which should fix width issues).

prymitive · 2021-04-15T10:13:49Z

I need to move html {} style out of bootstrap selectors

juliusv · 2021-04-15T10:26:22Z

I need to move html {} style out of bootstrap selectors

@prymitive Does that mean I shouldn't merge yet?

html block is root document so styles for it cannot be nested under theme classes. Move it out and add a bit of documentation to explain what which file does. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

prymitive · 2021-04-15T10:32:48Z

I've noticed that there's a rule for html {} and that wouldn't be correctly applied from _shared.scss.
Just added an extra commit that moves it

juliusv · 2021-04-15T12:10:40Z

Nice. One last nit I found before merging: the icons in buttons (metrics explorer button or time control buttons) were a couple of pixels higher in the existing UI. It seems that this styling for the icons got lost, and now they are a tad too low:

.svg-inline--fa {
    display: inline-block;
    font-size: inherit;
    height: 1em;
    overflow: visible;
    vertical-align: -0.125em;
}

juliusv · 2021-04-15T12:11:30Z

(compare https://deploy-preview-8604--prometheus-react.netlify.app/graph and https://prometheus.demo.do.prometheus.io/graph and switch between the two tabs to see the difference)

prymitive · 2021-04-15T12:42:13Z

This is caused by reboot classes being included in both themes in https://github.com/ForEvolve/bootstrap-dark
Reboot is to reset all browsers defaults to same styles, but here it overrides some styles applied by FontAwesome, might need to customise ForEvolve/bootstrap-dark builds to remove it from themes and apply only once for root documents.

Also raised ForEvolve/bootstrap-dark#49

Both bootstrap themes we use import reboot classes (https://getbootstrap.com/docs/4.6/content/reboot/) which has the side effect of overriding other classes. We need reboot to be applied as defaults for the browser, so it needs to be moved out of theme class selectors. But because reboot requires scss variables we need to feed it something, for that we use the default light theme, so it gets imported there and browser will use style of the default theme to reset default (unthemed) styles. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

prymitive · 2021-04-15T13:26:49Z

Added a workaround for reboot issues

This needs to be applied globally, not per theme. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

juliusv · 2021-04-15T16:14:02Z

👍 Nice, thanks for filing that issue as well!

I'll merge this for now, and then we can individually file follow-up improvement PRs to the colors.

Thanks again for all the work! :)

prymitive · 2021-04-15T16:18:52Z

Feel free to assign me tickets for any spotted issues.

juliusv · 2021-04-15T16:27:41Z

@prymitive FYI https://twitter.com/juliusvolz/status/1382732267743809538

prymitive mentioned this pull request Mar 15, 2021

Implement a dark theme in the react UI #7735

Closed

prymitive force-pushed the dark-theme branch from f6b1c23 to 19a9571 Compare March 15, 2021 19:56

prymitive force-pushed the dark-theme branch from 19a9571 to cff4d65 Compare March 16, 2021 17:50

prymitive force-pushed the dark-theme branch from cff4d65 to 2234466 Compare March 20, 2021 21:06

prymitive force-pushed the dark-theme branch 2 times, most recently from f95afc5 to 50b52ad Compare March 21, 2021 16:07

prymitive marked this pull request as ready for review March 21, 2021 16:54

prymitive requested a review from juliusv as a code owner March 21, 2021 16:54

juliusv reviewed Mar 22, 2021

View reviewed changes

prymitive force-pushed the dark-theme branch from bcd6ea4 to cc3acd7 Compare March 24, 2021 10:45

prymitive force-pushed the dark-theme branch from cc3acd7 to 012643d Compare March 24, 2021 10:49

juliusv reviewed Mar 24, 2021

View reviewed changes

prymitive added 12 commits April 15, 2021 10:06

Add a dark theme for CollapsibleAlertPanel

2b6ec2c

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add a dark theme for RulesContent

1835a70

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add a dark theme for Config

a2cb466

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Use bootstrap classes for margins

a33c6d4

We can override margins via bootstrap css classes instead of loading custom css module. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add a dark theme for QueryStatsView

4e22eb1

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add a dark theme for MetricsExplorer

12cff37

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add dark theme for Graph panel input

54535bb

This makes the whole input group look consistent in dark mode as the old styles were made to blend it with the default bootstrap theme. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Add a dark theme for /graph page tabs

54b8cf0

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Fix metrics explorer modal scroll

19b5330

bootstrap-dark breaks scrolling on the metrics modal, so we need an extra rule to fix that. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Move App.css rules to themes/_shared.scss

2dc3893

This completes splitting styles into light and dark theme. It also fixes some small issues with themes as now all styles from App.css are applied correctly. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

prymitive force-pushed the dark-theme branch from 3214e0f to 2dc3893 Compare April 15, 2021 09:31

Move html{} styles to a dedicated file

be3cf97

html block is root document so styles for it cannot be nested under theme classes. Move it out and add a bit of documentation to explain what which file does. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

Move codicon font to app.scss

11f290e

This needs to be applied globally, not per theme. Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>

juliusv merged commit 850dbda into prometheus:main Apr 15, 2021

prymitive deleted the dark-theme branch April 15, 2021 16:17

Aman-Codes mentioned this pull request May 16, 2021

Implement a dark theme in UI thanos-io/thanos#3775

Closed

	title="Use browser preferred theme"
	title="Use browser's preferred theme"

	title="Use browser preferred theme"
	title="Use browser-preferred theme"

Add a dark theme #8604

Add a dark theme #8604

Conversation

prymitive commented Mar 15, 2021

roidelapluie commented Mar 15, 2021

prymitive commented Mar 15, 2021

roidelapluie commented Mar 15, 2021

prymitive commented Mar 15, 2021

juliusv commented Mar 15, 2021

juliusv commented Mar 15, 2021

juliusv commented Mar 15, 2021

prymitive commented Mar 16, 2021

prymitive commented Mar 16, 2021

juliusv commented Mar 17, 2021

prymitive commented Mar 17, 2021

prymitive commented Mar 20, 2021

prymitive commented Mar 20, 2021

prymitive commented Mar 21, 2021

juliusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Mar 24, 2021

prymitive commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Mar 24, 2021

juliusv commented Mar 24, 2021

prymitive commented Apr 15, 2021

prymitive commented Apr 15, 2021

juliusv commented Apr 15, 2021

prymitive commented Apr 15, 2021

juliusv commented Apr 15, 2021

juliusv commented Apr 15, 2021

prymitive commented Apr 15, 2021 • edited

prymitive commented Apr 15, 2021

juliusv commented Apr 15, 2021

prymitive commented Apr 15, 2021

juliusv commented Apr 15, 2021

prymitive commented Apr 15, 2021 •

edited