[Bug] Fix theme flickering and 404 page

[huong-li-nguyen]

Description

• Fixes Fix theme flickering #866 and 404 page appears unstyled #834

Antony:
Almost certainly caused by #804. Note that the theme selector callback doesn't > run on this page. Just adding data-bs-theme="dark" in devtools fixes it so I'm sure this is easy to fix, but I'm curious > why it doesn't work already given that we set :root to be the same as that?

The reason why this didn't work, despite setting the root, is due to several factors:

1. No data-bs-theme attribute: The background color of the 404 page was set using background: var(--surfaces-bg-03);. This CSS variable is only available when a data-bs-theme is assigned. Since this wasn't the case, the variable couldn't be found and it defaulted to bs-body-bg. This issue has been addressed in this PR ✅.

2. Incomplete Bootstrap Styling: While we styled selected components in our Bootstrap theme, we didn't add styling for the body, etc. Therefore, even if the root is set to match data-bs-theme, it has no effect because bs-body-bg defaults to the original Bootstrap color. This has been fixed in this PR ✅ .

3. Theme Order in Bootstrap: To rely on root and its variables, we needed to change the order of the Bootstrap themes in vizro-bootstrap.min.css. Currently, our Vizro Bootstrap theme is listed first, followed by the general Bootstrap theme. As a result, the valid bs-body-bg value is taken from the general Bootstrap theme instead of ours. This issue has also been fixed in this PR ✅ , but I would like @pruthvip15 to double-check if the changed order is correct.

An alternative and potentially better visual solution would be to assign the data-bs-theme attribute directly to the 404 page and possibly the loading page as well. This would prevent inconsistencies where the 404 and loading pages might have a different overall theme than the selected one (e.g., if the default is light). Is that possible?

Screenshot

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.

[github-actions]

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-11-13 13:32:04 UTC
Commit: f8bfda4

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

[petar-qb]

I like the changes and it would be even better if the theme of the 404 page matched the one currently selected (eg light theme).

I have one general question: Do we still need the loading page?

As I can remember, the loading page was added to avoid a blank white screen when opening a page that visualises big data.
And, after this PR is merged, there should not be any blank white screen problems anymore. I can't remember all those stuff clearly, hence I don't have strong opinion on that.

[antonymilne]

Thanks for fixing this so quickly! 🙏

An alternative and potentially better visual solution would be to assign the data-bs-theme attribute directly to the 404 page and possibly the loading page as well. This would prevent inconsistencies where the 404 and loading pages might have a different overall theme than the selected one (e.g., if the default is light). Is that possible?

Yeah, this is kind of what I was expecting the fix to be. It's possible a few ways but remains to be seen how effective they are...

Now I look at the code again probably the best solution, if it's possible, would be to do this in Vizro.build:

self.dash.index_str = self.dash.index_str.replace("<html>", f"<html data-bs-theme={dashboard.theme}")

Does that work?

Next best option would maybe be this code in a Vizro assets folder that gets executed when used as a framework:

document.documentElement.setAttribute('data-bs-theme', 'dark')

[huong-li-nguyen]

this PR

The first point depends on whether we can set the data-bs-theme attribute for both the loading-page and the 404 page. I'll try out what @antonymilne wrote below in a sec! :)

On the loading-page, from what I remember @maxschulz-COL added this is, because there was a use case with an internal team where the general dashboard took a bit longer to load and the default white page with the loading icon was too ugly 😄 I think this is still the case, hence would keep the page. But correct me if I am wrong @maxschulz-COL

[huong-li-nguyen]

Thanks for fixing this so quickly! 🙏

An alternative and potentially better visual solution would be to assign the data-bs-theme attribute directly to the 404 page and possibly the loading page as well. This would prevent inconsistencies where the 404 and loading pages might have a different overall theme than the selected one (e.g., if the default is light). Is that possible?

Yeah, this is kind of what I was expecting the fix to be. It's possible a few ways but remains to be seen how effective they are...

Now I look at the code again probably the best solution, if it's possible, would be to do this in Vizro.build:

self.dash.index_str = self.dash.index_str.replace("<html>", f"<html data-bs-theme={dashboard.theme}")

This works to fix the original issues, but I think we would have to add the bootstrap changes anyway :) However, it doesn't fix the visual inconsistency, that the 404 page and loading page might have different themes if the light theme is the default. This can be fixed if we also add the toggle-switch to the 404 page and the loading page. It's possible for the 404 page, but I don't see a way for the loading-page.

[maxschulz-COL]

this PR

The first point depends on whether we can set the data-bs-theme attribute for both the loading-page and the 404 page. I'll try out what @antonymilne wrote below in a sec! :)

On the loading-page, from what I remember @maxschulz-COL added this is, because there was a use case with an internal team where the general dashboard took a bit longer to load and the default white page with the loading icon was too ugly 😄 I think this is still the case, hence would keep the page. But correct me if I am wrong @maxschulz-COL

@petar-qb @huong-li-nguyen
The internal team is currently not working on this anymore, so the specific case may have vanished. But if I remember correctly, this fixed two things:

• avoid blank screen on slow loading pages (should ideally not be the case anymore after taking out the data from the page build)
• avoid blank screen before dash renderer has even kicked in - this is not really related to the app itself, and since our default theme is dark we made that dark as well, so that there would be not change from white to dark

For the second reason I am not sure if anything changed in this PR so that we would not need it anymore. So if the page is dark before dash renderer kicks even without the loading page we can get rid of it, otherwise I would keep it!

[antonymilne]

Approved subject to various changes 🙂 The key thing is that the data-bs-theme should match dashboard.theme rather than being hardcoded to dark. Thank you for fixing this!

In response to @petar-qb and @maxschulz-COL points about the loading spinner:

• the significance of the spinner is that it shows instead of the Dash "Loading..." screen. Before we customised this, even for quickly loading apps this was always present before and with a fixed white background which caused a slight flicker
• we still need to customise this page from the Dash default somehow to avoid the white flash. A rotating spinner isn't really needed any more since the screen can no longer show for a long time, but having the right coloured background and not looking too gross is important
• the best we can easily do here is just set the background colour to match dashboard.theme. This won't take account of the user-selected theme but will reflect the dashboard developer's preferred theme. Getting it to match the user-selected theme is probably possible but a lot harder and not worth the effort.
• once we have a matching background colour, so long as the "Loading..." page doesn't show for long (hopefully it never does any more) then it doesn't really matter what it looks like. Either the loading spinner or just a blank page is fine by me

[petar-qb]

I agree with the Antony's comment. I would also get rid of the loading spinner from the loading-page and I'd with stick with the coloured page only.

Currently, the loading page background colour is always dark. If we can align it with the user's selection (theme-selector) easily then it would be the perfect solution. If there's no way to do so easily, then setting the loading-page background colour to the Dashboard.theme would also be a good enough solution in my opinion.

Thanks for handling 404 page, it is really a slick solution 👍

[maxschulz-COL]

I agree with the Antony's comment. I would also get rid of the loading spinner from the loading-page and I'd with stick with the coloured page only.

Currently, the loading page background colour is always dark. If we can align it with the user's selection (theme-selector) easily then it would be the perfect solution. If there's no way to do so easily, then setting the loading-page background colour to the Dashboard.theme would also be a good enough solution in my opinion.

Thanks for handling 404 page, it is really a slick solution 👍

I would just note that I see the spinner quite often (albeit shortly), basically on any app that I open. I like it (but I am not married to any design). I feel having a spinner shows a level of user-reassurance that things are happening, even if it always is short to show. Having the background match dashboard.theme is of course a great addition.

But also not too married to any decision, just my 5 cents here :)

[huong-li-nguyen]

I would just note that I see the spinner quite often (albeit shortly), basically on any app that I open. I like it (but I am not married to any design). I feel having a spinner shows a level of user-reassurance that things are happening, even if it always is short to show. Having the background match dashboard.theme is of course a great addition.

But also not too married to any decision, just my 5 cents here :)

I tried refreshing the page and using some apps that take longer, and I decided to get rid of the spinner. It was just showing up for such a short time that it didn't seem worth it. I agree, it's always good to let users know something is happening, but in this case, the spinner was more distracting because it flashed on and off so quickly.

For almost all of the page refreshes, there was always the chart spinner visible, and afterwards came the loading spinner. Thus, it showed two different screens in such a short amount of time, so getting rid of one of the spinners is actually nicer! For the initial app load, I agree it would be nice, but given that you cannot differentiate between these two cases, I would just leave it 👍

[petar-qb]

Great work!!

Do you mind opening a ticket to match the background colour of the loading-page with the currently selected theme colour, just to make sure we don't forget this very small improvement that would complete these UX enhancements. If you think it's irrelevant, I'm totally fine with that.

[huong-li-nguyen]

I think it's not worth doing - the UX improvement compared to the added complexity in code to enable this is probably not worth it 😄 Taking on the dashboard.theme is a good enough improvement for me 👍