mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.66k stars 137 forks source link

[POC] Investigate whether we can get rid of outer container with className to use vizro-bootstrap #732

Open huong-li-nguyen opened 2 weeks ago

huong-li-nguyen commented 2 weeks ago

Currently, if someone creates a pure Dash app and wants to use the vizro-bootstrap CSS file, they have to wrap their Dash app content inside an outer div with the className=vizro_light or vizro_dark.

If not provided, it does not take on the styling of our Vizro bootstrap stylesheet. Take this minimal example: https://github.com/mckinsey/vizro/blob/poc/add-example-bootstrap/vizro-core/examples/scratch_dev/app.py#L54-L55

@pruthvip15 - could you investigate the following?

@pruthvip15 - you can take the branch and dev example from above :)

I think because we define this in our bootstrap theme, we need this outer container, so I am not sure if there is any way how this can be read in without having to specify this out container

.vizro_dark, [data-bs-theme="dark"] {
...

}
pruthvip15 commented 2 weeks ago

Hey Yes, We can do this.

If we assume the default theme is dark, then we can change the above to

:root, .vizro_dark, [data-bs-theme="dark"] {
...

}

And this will ensure that by default the dark mode will be applied.

If the user wants to put light mode, then they have to explicitly wrap the container in .vizro_light or [data-bs-theme="light"]

I have made a small example here: https://codepen.io/pruthvip15/pen/mdNJXpm

In the above example, we can see that the default the app is in dark mode, if for a inner container the user wants light mode, they can add the respective class to it

huong-li-nguyen commented 2 weeks ago

Ahh, that's great to know! @antonymilne - would above suffice? Then people only have to specify vizro_light if they want the light theme.

antonymilne commented 5 days ago

Thanks for this @pruthvip15. I think this sounds good but will need a slight modification to be exactly right on Vizro - let me just add some more notes and next steps here.

Looking at https://bootswatch.com/5/darkly/bootstrap.css as an example, they do :root just like @pruthvip15 suggested:

:root,
[data-bs-theme=light] {
  ...
}

[data-bs-theme=dark] {
  color-scheme: dark; /* Looks useful since it changes colour in browser e.g. of spellcheck underline */
   ...
}

I think we should do the same on Vizro framework:

Pure Dash consumers of Vizro bootstrap would then do external_stylesheets=[...] just like normal consumption of a dbc theme. And if they want to switch between dark/light theme they'd follow the usual data-bs-theme specification like on https://hellodash.pythonanywhere.com/adding-themes/color-modes. No need for vizro_light or vizro_dark class names anywhere.

The only thing I'm not too sure about is whether for Vizro bootstrap we should make the default :root correspond to light or dark theme. On the one hand, dark is our "flagship default". But it seems to be common (maybe even necessary?) in bootswatch themes to have the light one as default. Even for the case of Darkly theme it looks like their light version of the theme is still the default. @AnnMarieW do you know why this is? 🤔 This page states that light is the default in bootstrap but I don't know if that rule should apply to all bootstrap themes too?

(Regardless of whether bootstrap theme :root goes with light or dark, this doesn't change anything on Vizro framework since theme is always controlled by Dashboard.theme and the switcher.)

AnnMarieW commented 1 day ago

Hi @antonymilne

I missed your mention earlier. Yes, the Bootstrap Color Modes assumes a light theme as a default, then defines the changes needed for the dark mode. This doesn't work well when you start with a default dark theme like the Bootstwatch Cyborg or Darkly. I'm not sure if they plan on defining a light mode for those themes.