-
Notifications
You must be signed in to change notification settings - Fork 144
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
[Tidy] Add vizro-bootstrap
stylesheet and remove custom CSS
#384
Conversation
for more information, see https://pre-commit.ci
…sey/vizro into tidy/replace_css_bootstrap
vizro-bootstrap
CSS file and remove custom CSS
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
… tidy/replace_css_bootstrap
… tidy/replace_css_bootstrap
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 is AMAZING work, thank you @huong-li-nguyen and @pruthvip15 also who I know has been very instrumental behind the scenes here 🚀 This is a big step forward I think and I know it's been a lot of work.
For now I'm happy to stick with option 1 like you have here where we just serve the stylesheet locally because that's easy and always works. Then we can figure out later how to handle CDN, serve_locally
option, whether to provide a vizro.themes.stylesheet
shortcut like bootstrap does, whether to try and put it in bootswatch/dbc, etc. etc.
I guess there might be a few follow on PRs like this while I'm on holiday - feel free to merge them all if they're just doing similar work like you're doing here, no need to wait for my review 🚀
vizro-bootstrap
CSS file and remove custom CSSvizro-bootstrap
stylesheet and remove custom CSS
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 looks really good. Awesome job! 🚀 🙌
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.
Looks really good!! 🚀
I like the way it's ensured that vizro-bootstrap.min.css is loaded first to allow overwrites 😄.
Description
Accordion
,Button
,Card
,Typography
,NavBar
Button
,Card
,NavBar
,NavLink
,For Users
This change should only affect your app if you have been using custom CSS. If you've been using custom CSS and referenced any CSS classNames that have now been removed from our stylesheet, your components might look off. In this case, you need to target the correct Bootstrap CSS classNames instead.
For Reviewers
Don't be scared of the big number of added code lines. This is mainly due to adding the
vizro-bootstrap.min.css
as a physical file into our repository, which you don't have to review. In the long term, this file might be hosted elsewhere, and then we need to link the URL to our external stylesheets (1 line).Context
The
vizro-bootstrap.min.css
is the output of the following private repository: https://github.com/McK-Private/vizro-bootstrap. It's currently private, but we might also open-source it (not in any near PR). Currently, it's a WIP version where the CSS is created using SASS (for context: https://blog.hubspot.com/website/how-to-override-bootstrap-css#how-to-customize-bootstrap-sass). The vizro-bootstrap CSS file does not contain the correct design for all components. Instead, we will gradually extend it.What does it mean for our future development?
1) Preferred choice of components: Always prefer the
dbc
component if the functionality is the same. In some cases, it makes sense to deviate from that choice, e.g., if the component is not available in dbc or functionality is significantly better in the other library. In this case, custom CSS needs to be added (short-term), but even better, a translation layer for CSS needs to be created to map bootstrap properties to CSS classNames of another component library.2) Adding custom CSS: No custom CSS should be added anymore when adding new components. Everything should go via the bootstrap theme, so we have one single source of truth. In some cases, it might require custom CSS, but this needs to be decided on a case-by-case basis. The general guideline is that all CSS needs to be created via the Bootstrap theme.
@antonymilne - I think we have two options in terms of the current deployment
vizro-bootstrap.min.css
inside the static folder of vizro-core and load the static file into the external stylesheets (as currently in this PR)vizro-bootstrap.min.css
outside of vizro-core, so it's not part of the package, e.g., inside a folder calleddist
. The advantage of this approach is that it doesn't increase the size of our package, and we can still reference the CSS file via a CDN link: https://medium.com/javarevisited/how-to-host-your-repository-js-css-on-open-source-cdn-jsdelivr-4de252d6fbadI have a slight preference for 1) Now after reading through the arguments on why Dash decided to server their assets locally as a default: plotly/dash#469
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":