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

[Feat] Improve background color and loading spinner on loading layout #598

Merged
merged 17 commits into from
Aug 2, 2024

Conversation

maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Jul 25, 2024

Description [edited]

This PR contains:

  • background color change while Dash is loading AND while layout is building
  • change the Loading... text to a loading spinner

Given that we do not have access to our variables yet, it was decided to choose a hard-coded dark mode color in root.

Screenshot

image

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.

@maxschulz-COL maxschulz-COL marked this pull request as ready for review July 25, 2024 09:39
@maxschulz-COL maxschulz-COL self-assigned this Jul 25, 2024
@pre-commit-ci pre-commit-ci bot temporarily deployed to circleci_secrets July 25, 2024 09:40 Inactive
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.

The code looks good but it doesn't actually seem to work for me... Maybe we're being too fancy with the spinner and should just change the colour of the background of the screen. Just glancing at the dev tools, maybe we need to worry about _dash-loading-callback instead/as well as _dash-loading? Not sure.
https://github.com/user-attachments/assets/d83b3725-c219-4219-93f2-6c8366e3d442

@antonymilne
Copy link
Contributor

Hmmm, I guess this is because of the Dash pages callback that runs to update the page contents 🤔 Maybe we need to do this instead or as well? https://community.plotly.com/t/displaying-loading-screen-when-pages-container-is-loading/72109
https://dash.plotly.com/loading-states
But also we don't want this to activate when we change pages, so maybe this is going to be impossible to fully achieve in a sensible way after all :/ Not sure.

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Jul 25, 2024

Hmmm, I guess this is because of the Dash pages callback that runs to update the page contents 🤔 Maybe we need to do this instead or as well? https://community.plotly.com/t/displaying-loading-screen-when-pages-container-is-loading/72109 https://dash.plotly.com/loading-states But also we don't want this to activate when we change pages, so maybe this is going to be impossible to fully achieve in a sensible way after all :/ Not sure.

What do you mean by this @antonymilne? I don't fully follow 😄 For me the dev example worked but there was also only one page. For me it looked like it works fine. What potential issues do you anticipate?

@huong-li-nguyen huong-li-nguyen changed the title [Feat] Improved background color and loading spinner on loading layout [Feat] Improve background color and loading spinner on loading layout Jul 25, 2024
@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Jul 25, 2024

Description

This PR changes two things:

  • background before the apps layout loads
  • change the Loading... text to a loading spinner

Given that we do not have access to our variables yet, it was decided to choose a hard-coded dark mode color in root.

Would eventually rename to [Feat] Improve landing waiting page before app load to make it clearer?

@antonymilne
Copy link
Contributor

What do you mean by this @antonymilne? I don't fully follow 😄 For me the dev example worked but there was also only one page. For me it looked like it works fine. What potential issues do you anticipate?

The thing I'm not sure about basically comes down to "where do I put my time.sleep(5) in order to see the effect this change has? If I put it in page.build then you get the movie I showed above. But I'm not sure there's any good way to fix that. So where is the right place to put time.sleep in order to get a "Loading..." screen that shows for a while?

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Jul 25, 2024

What do you mean by this @antonymilne? I don't fully follow 😄 For me the dev example worked but there was also only one page. For me it looked like it works fine. What potential issues do you anticipate?

The thing I'm not sure about basically comes down to "where do I put my time.sleep(5) in order to see the effect this change has? If I put it in page.build then you get the movie I showed above. But I'm not sure there's any good way to fix that. So where is the right place to put time.sleep in order to get a "Loading..." screen that shows for a while?

I've put it inside the chart. It will only extend the loading screen for a few seconds, but sufficiently enough to see how it looks like. At least it was enough for me to edit the CSS above 😄 It should also be better visible if we implement the code suggestion above :) I made it a bit bigger and placed it at the center of the page. The animation I think is also slower. Would still like to have @pruthvip15 take a look at the CSS on the code suggestion above.

"""Example app to show all features of Vizro."""

import pandas as pd
import vizro.models as vm
import vizro.plotly.express as px
from vizro import Vizro
from vizro.models.types import capture
import time
gapminder = px.data.gapminder()

@capture("graph")
def boxplot(data_frame):
    time.sleep(20)
    return px.box(data_frame, x="continent", y="lifeExp", color="continent")


home = vm.Page(
    title="Page Title",
    components=[
        vm.Graph(figure=boxplot(gapminder)),
    ],
)


dashboard = vm.Dashboard(pages=[home])


if __name__ == "__main__":
    Vizro().build(dashboard).run()

@maxschulz-COL
Copy link
Contributor Author

maxschulz-COL commented Jul 25, 2024

What do you mean by this @antonymilne? I don't fully follow 😄 For me the dev example worked but there was also only one page. For me it looked like it works fine. What potential issues do you anticipate?

The thing I'm not sure about basically comes down to "where do I put my time.sleep(5) in order to see the effect this change has? If I put it in page.build then you get the movie I showed above. But I'm not sure there's any good way to fix that. So where is the right place to put time.sleep in order to get a "Loading..." screen that shows for a while?

So I had a similar problem! I never really resolved it, but it was also hard to track down the root cause because most of that work was done on a train where it struggled even to load the static google assets 😂 . In a pure dash app it is easy to make the layout loading function have a sleep timer as in this example

What I do is I pause the app in the Sources tab of the inspect console - like that it pauses on that frame at least!

@huong-li-nguyen huong-li-nguyen self-requested a review July 25, 2024 14:31
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.

Really nice work, and a great team effort! 🙌 The white loading screen has always annoyed me so nice to have it gone 🥳

I spoke with @maxschulz-COL about this already - there's still a small white flash in between the loading spinner disappearing and the page appearing. I don't know whether it will be possible to get rid of that, but do give it a quick go and see if targeting html can do it without breaking the page after it's finished loading.

To clarify some of the things I said earlier and also explain what's happening here, since it is pretty confusing as there's many different things that load on our page, and this will become important to understand in discussions we'll have soon about actions on-page-load etc.

This is what's in the HTML of a Dash app:

<div id="react-entry-point">
    STUFF
</div>

The order of page loading is something like this:

  1. Dash runs app.layout = dashboard.build() on backend to set app.layout, which is passed to Dash renderer
  2. while this is happening, STUFF above is <div class="_dash-loading">...</div>
  3. when it's finished, STUFF becomes the real page content, i.e. what's returned by app.layout
  4. for us, this app.layout is an html.Div that contains a few things including dash.page_container
  5. dash.page_container is initially an empty placeholder but populated by the Dash pages callback
  6. for us, this page_container sets layout=partial(self._make_page_layout, page), which ends up calling page.build
  7. since this is set as a function, every time you refresh the page, this layout function is re-run (but the overall dashboard.build is not)
  8. when you change page, the Dash pages callback executes but does not do a full page refresh, so steps 1 and 2 above do not apply
  9. for us it's even more complicated 😅 Because the page that gets loaded by the Dash pages callback just contains blank placeholders for the graph and table/grid (not currently the case, but ideally would be) and those then get populated with the real data in the on_page_load action

The white "Loading..." screen only shows during steps 1 and 2 above and not after that. Hence the correct way to get this to show is to delay these two steps. Putting a time.sleep in page.build like I was trying doesn't do it, and neither does putting it inside the graph function like @huong-li-nguyen does above. These both delay steps that happen after 1/2 and do not affect the "Loading..." screen.

There's at least 3 ways to delay steps 1/2:

# The code to sleep for 5 seconds is not good JS but it works fine for this example
hooks = {
    "layout_pre": """
           () => {
                var date = new Date();
                var curDate = null;
                do { curDate = new Date(); }
                while(curDate-date < 5000);
                console.log("pre")
                                      
           }
       """,
    "layout_post": """
           (response) => {
                var date = new Date();
                var curDate = null;
                do { curDate = new Date(); }
                while(curDate-date < 5000);
                console.log("post")
                console.log(response)
              }
       """,
}
self.dash = dash.Dash(**kwargs, use_pages=True, pages_folder="", title="Vizro", hooks=hooks)

vizro-core/src/vizro/static/css/loading.css Show resolved Hide resolved
vizro-core/src/vizro/static/css/loading.css Outdated Show resolved Hide resolved
@huong-li-nguyen
Copy link
Contributor

Thanks for the explanation! 🚀

I just went through these docs, which might be helpful. Firstly, you can customize the "Loading..." message. Referring back to @pruthvip15's suggestion, instead of making the message transparent, we should remove it if possible. This can be done via:

app = Dash(__name__, update_title=None)

You can also customize the Dash index page: https://dash.plotly.com/external-resources#customizing-dash's-html-index-template

I'm not sure if this will help in this specific case, but you could potentially make it dark by ensuring some of your CSS loads before Dash's CSS. However, I'm not sure if it's worth the effort. The transition from the white to dark loading screen only lasts a split second, so it doesn't bother me that much.

@antonymilne
Copy link
Contributor

@huong-li-nguyen AIUI update_title doesn't affect what displays on the "Loading..." screen. It only affects the title of the browser tab while a callback is running.

I think we should set update_title=None anyway because I think the Dash default behaviour here ("Updating...") isn't particularly useful for our needs. We can do this in this PR because it's a tiny change, but I'm pretty sure it's completely separate from the loading screen.

(Yes this is all very confusing - this is what I meant above that there's many different things in Dash that are to do with loading.)

Agree that editing the index template isn't worth doing just to try and eliminate the small white flash - it also doesn't bother me much. So I would only do this if it's a simple bit of extra CSS like we're doing here, not if it's any harder than that.

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.

It looks like the white flash has gone?! Amazing work!! 🙌

@maxschulz-COL maxschulz-COL merged commit 3d23067 into main Aug 2, 2024
30 checks passed
@maxschulz-COL maxschulz-COL deleted the feat/improved_load_CSS branch August 2, 2024 12:25
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