-
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] Remove all CSS variables and tokens #886
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-11-26 12:38:28 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
for more information, see https://pre-commit.ci
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.
Love love love this 😀 I know it's been an epic task, thank you seeing it through!
I didn't actually try anything out but I trust our screenshot tests to catch if anything is wrong.
vizro-core/changelog.d/20241119_173819_huong_li_nguyen_remove_variables.md
Show resolved
Hide resolved
I had to update some of them as well 😄 But don't worry, I went through min. 10-15 rounds of screenshot comparisons to check where the difference lies. Some of our token values have changed in the QB design library, but that change didn't go through to Vizro because our tokens don't automatically update 👍 |
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.
LGTM from a docs perspective
for more information, see https://pre-commit.ci
…izro into tidy/remove-variables
for more information, see https://pre-commit.ci
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.
Well done! Looks very sensible and useful. Not being a CSS expert I feel like after re-reading the docs I know what to do. I understood:
- Start with playing with properties, changing them in the elements panel etc
- Move to bootstrap variables, or define your own variables for a more comprehensive, stable solution where you have things react to theme switches
If that understanding is correct, then I think we are good!
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.
LGTM 🏆 ⭐ 🏅
Description
For Developers:
For any upcoming CSS modifications, please use the CSS variables from
vizro-bootstrap.min.css.
Your code editor should offer autocompletion so you can see the available options. Avoid using any outdated variables/tokens.Impact on Demos:
Be aware that all demos using custom CSS and previously relying on our static folder's CSS variables/tokens will be affected. This primarily impacts our internal demos, though there is a slight possibility it could affect a user demo (e.g. if we provided them with custom CSS snippets that used our variables/tokens).
To update our demos, we need to merge this PR first and then release
vizro
. Following this, we can merge this upcoming PR and update thevizro
version accordingly.Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":