-
Notifications
You must be signed in to change notification settings - Fork 153
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
[Bug] Fix coloring of NavLinks inside NavBar #968
Conversation
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2025-01-24 09:40:54 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
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.
Great spot! Thanks for fixing it.
However, I found that this change broke the "Made with Vizro" banner look.
Dark theme | Light theme |
---|---|
![]() |
![]() |
I'm also wondering how our screenshot tests didn't catch the incorrect NavLinks colour (introduced in one of our previous PRs) or the appearance of the "Made with Vizro" banner (that's introduced with this PR). @l0uden do we have screenshot tests that check NavLinks or "Made with Vizro" banner? Maybe it doesn't make sense to make a check for the banner as it's a custom component added in some of our examples. 🤔
Ah, great catch! 🚀 Fixed now. I don't think we have any screenshot tests for the banners, as these only appear in our demos and our demos are fixed to a specific vizro version on HF (so that bug wouldn't have affected the banners unless we upgraded the vizro version). |
FYI: I just had a quick chat with @l0uden and he'll add a banner and navlink into an existing screenshot test that checks both themes. |
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 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.
Checking the demos this looks good, thanks!
Description
With the latest Bootstrap theme, the NavLinks inside our NavBars were incorrectly colored. However, this is actually not an issue of the Bootstrap theme, as the CSS itself is correct.
The issue appears as with the usage of
dbc.NavBar
, the className ="navbar-light" is automatically attached to that component in both dark and light theme. We could probably fix this via callbacks, but I think the CSS fix is fine.Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":