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

Add default Bootstrap theme and overwrite CSS #311

Merged
merged 33 commits into from
Feb 22, 2024

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Feb 14, 2024

Description

This is also just a temporary POC fix to an issue we will solve differently long-term, changes included in this PR:

  • Add default Bootstrap theme
  • Overwrite relevant CSS properties such that the Bootstrap theme has no effect on styling
  • Remove CSS code that is not required anymore (as functionality is shipped automatically with Bootstrap theme now)
  • Updated screenshots in the vizro-qa repo as there are new defaults for some elements

The current approach is not as different as our previous one, but has a few advantages:

Remains the same:

  • dbc.themes still do not work properly in our code basis (in the sense of switching skins)
  • When adding a dbc.component to our code basis we still need to use custom CSS to apply our design (eventually we need to overwrite more properties now)

Improvement to previous version:

  • Visual consistency: Plugging a dbc.theme will have no visual effect now (with the currently existing components), as our CSS overwrites their CSS
  • Less code to maintain:
    • Including a dbc.theme by default has the advantage that all relevant CSS for the proper functioning of dbc components are correctly shipped, so when someone adds a dbc component as a custom component, it will work out of the box and they don't have to add CSS code to their assets folder anymore (same applies to us)
    • Many html elements that we did not have stylings for (e.g. ul, li, etc.) have a default Bootstrap styling applied now.

Screenshot

When plugging in a dbc.theme before:
Screenshot 2024-02-14 at 20 46 34
Screenshot 2024-02-14 at 20 46 39

Now:
Screenshot 2024-02-14 at 20 47 10

Screenshot 2024-02-14 at 20 47 02

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@huong-li-nguyen huong-li-nguyen self-assigned this Feb 14, 2024
@huong-li-nguyen huong-li-nguyen changed the title Bug/fix bootstrap plugin Add default Bootstrap theme and overwrite CSS Feb 14, 2024
@huong-li-nguyen
Copy link
Contributor Author

huong-li-nguyen commented Feb 19, 2024

@antonymilne - this was initially supposed to be just a POC but shall we go ahead with this PR change now? Main advantages for me are:

  • Lots of CSS functionality shipped automatically now for dbc.components (benefits us and users that add dbc.components as custom components)
  • Default styling applied to semantic html elements that we haven't styled yet (e.g. a, li, ul, etc.)
  • When someone does add a Bootstrap theme, in theory it shouldn't look completely off anymore as it just gets ignored as our CSS overwrites the Bootstrap ones for the components that are currently present
  • Good exercise in general to see which CSS we have to overwrite for our components (would be nice to see it immediately when adding a new component now)

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

This seems like a big step in the right direction to me and I think we should go ahead with it 👍

If we can now remove the "Adding a dash-bootstrap component" from the docs then let's do so. The "Dash Bootstrap Themes" warning is still relevant but could maybe be modified a bit now we know the direction of travel:

    Please note that Vizro is not yet compatible with [Dash Bootstrap Themes](https://dash-bootstrap-components.opensource.faculty.ai/docs/themes/).
    Adding a Bootstrap stylesheet will not have any effect on your Vizro app.

vizro-core/src/vizro/_vizro.py Show resolved Hide resolved
vizro-core/src/vizro/models/_components/card.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/static/css/layout.css Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_components/card.py Outdated Show resolved Hide resolved
@huong-li-nguyen
Copy link
Contributor Author

This seems like a big step in the right direction to me and I think we should go ahead with it 👍

If we can now remove the "Adding a dash-bootstrap component" from the docs then let's do so. The "Dash Bootstrap Themes" warning is still relevant but could maybe be modified a bit now we know the direction of travel:

    Please note that Vizro is not yet compatible with [Dash Bootstrap Themes](https://dash-bootstrap-components.opensource.faculty.ai/docs/themes/).
    Adding a Bootstrap stylesheet will not have any effect on your Vizro app.

I've modified to below, but I think the warning is still relevant. However, I've removed the other warning as functionality should be automatically shipped now via the default dbc.theme included, and styling is required for any custom component that gets added by the user 👍

Please note that Vizro is currently not compatible with [Dash Bootstrap Themes](https://dash-bootstrap-components.opensource.faculty.ai/docs/themes/).
    Adding a Bootstrap stylesheet will have no visual effect on the [components](https://vizro.readthedocs.io/en/stable/pages/user_guides/components/) included in Vizro.

@huong-li-nguyen
Copy link
Contributor Author

@AnnMarieW - would you mind taking a look at this PR? I would love to get your opinion on it!

This is by no means our final solution to our Bootstrap issue. We are still not compatible with dbc.themes, but at least when someone tries to plug it in, it will have no visual effect on the currently existing components. So, I still kept the warning inside.

However, what did improve is that a dbc.theme is automatically attached right now, so at least anyone creating a custom component on dbc, doesn't have to figure out the custom CSS required to make the component functional.

@AnnMarieW
Copy link
Contributor

@huong-li-nguyen Nice work! This is awesome!

In dashboard.js if you update this callback function, it will will use the correct light/dark color mode for the Boootstrap theme:

export function _update_dashboard_theme(checked) {
  document.documentElement.setAttribute('data-bs-theme', checked ? 'light' : 'dark');
  return checked ? "vizro_light" : "vizro_dark";
}

@huong-li-nguyen
Copy link
Contributor Author

@huong-li-nguyen Nice work! This is awesome!

In dashboard.js if you update this callback function, it will will use the correct light/dark color mode for the Boootstrap theme:

export function _update_dashboard_theme(checked) {
  document.documentElement.setAttribute('data-bs-theme', checked ? 'light' : 'dark');
  return checked ? "vizro_light" : "vizro_dark";
}

Ahh, super smart! Thank you!!! 🚀 💯 Hope we can get a better long-term solution in place soon!

Copy link
Contributor

@nadijagraca nadijagraca left a comment

Choose a reason for hiding this comment

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

Looks good, I just have one minor suggestion but does not have to be included in this PR. 🚀

@huong-li-nguyen huong-li-nguyen merged commit 55c714c into main Feb 22, 2024
33 checks passed
@huong-li-nguyen huong-li-nguyen deleted the bug/fix_bootstrap_plugin branch February 22, 2024 09:22
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.

4 participants