-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add Loading Spinners to Components in home.py
#143
Conversation
CURRENTLY WIP: |
80d3e3f
to
0448023
Compare
Accidentally commited to wrong branch; tried to fix |
I've implemented skeleton loading for the app, but I’m unsure about one detail. Right now, some components load faster than others, creating a staggered loading experience. I’m considering whether it would be better to have all components wait for the slowest one to finish loading before displaying everything together. Visually, it seems more cohesive to have everything appear at once, but I’m concerned this might negatively impact perceived performance since faster components are delayed. Would love to hear your thoughts on which approach is better from a UX/design perspective: Staggered loading (as it is now) (the video is not up to date but skeleton is implemented) |
Yesterday we looked at both approaches. The skeleton loading looked great! |
def make_layout(): | ||
layout = html.Div([ | ||
html.Div(id='home-page-load', style={'display': 'none'}), | ||
dcc.Location(id='url', refresh=False), | ||
dcc.Store(id='store-trips', data={}), | ||
dcc.Store(id='store-uuids', data={}), | ||
dcc.Store(id='store-excluded-uuids', data={}), | ||
dcc.Store(id='store-demographics', data={}), | ||
dcc.Store(id='store-trajectories', data={}), | ||
html.Div(id='page-content', children=make_home_page()), | ||
|
||
]) | ||
logging.debug("Main layout component IDs: %s", get_all_component_ids(layout)) | ||
return layout | ||
|
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.
If the only purpose of this was for your own debugging, please revert
@@ -31,7 +31,7 @@ | |||
html.Div(id='tabs-content'), | |||
dcc.Store(id='selected-tab', data='tab-uuids-datatable'), # Store to hold selected tab | |||
dcc.Interval(id='interval-load-more', interval=20000, n_intervals=0), # default loading at 10s, can be lowered or hightened based on perf (usual process local is 3s) | |||
dcc.Store(id='store-uuids', data=[]), # Store to hold the original UUIDs data | |||
#dcc.Store(id='store-uuids', data=[]), # Store to hold the original UUIDs data |
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.
#dcc.Store(id='store-uuids', data=[]), # Store to hold the original UUIDs data |
Yes, you can remove this Store from this page. As you have correctly determined, the store-uuids
that comes from app_sidebar_collapsible.py
is global and redeclaring it here will cause issues
be in app.py. Since the dcc.Location component is not in the layout when navigating to this page, it triggers the callback. | ||
The workaround is to check if the input value is None. | ||
|
||
""" |
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 comment did not make sense or seem relevant to me either.
We looked back at the blame to see what commit it was added (6cf3d74#diff-e1d0f200a354a62599c454209b8b5815e380dd4ce4394c3e57a8f1a2e718113d) and did not find anything to justify keeping the comment.
So I am in support of removing the comment
@@ -62,7 +81,6 @@ def compute_sign_up_trend(uuid_df): | |||
) | |||
return res_df | |||
|
|||
|
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.
Try to minimize whitespace changes
Especially because IIRC, having 2 line breaks between functions is the PEP8 spec
@@ -385,4 +414,4 @@ def display_page(search): | |||
SESSION_COOKIE_HTTPONLY=True | |||
) | |||
logging.debug("after override, current server config = %s" % server.config) | |||
app.run_server(debug=envDebug, host='0.0.0.0', port=envPort) | |||
app.run_server(debug=envDebug, host='0.0.0.0', port=envPort) |
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.
whitespace change
from uuid import UUID | ||
from dash import dcc, html, Input, Output, callback, register_page | ||
from dash import dcc, html, Input, Output, callback, register_page, no_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.
It's not clear to me what no_update
does or why it's added. Can you add a comment in the code with an explanation (or a link to Dash documentation that explains it)?
@callback( | ||
Output('card-users', 'children'), | ||
Input('store-uuids', 'data'), | ||
) | ||
def update_card_users(store_uuids): | ||
number_of_users = store_uuids.get('length') if has_permission('overview_users') else 0 | ||
card = generate_card("# Users", f"{number_of_users} users", "fa fa-users") | ||
return card | ||
|
||
|
||
@callback( | ||
Output('card-active-users', 'children'), | ||
Input('store-uuids', 'data'), | ||
[ | ||
Output('card-users', 'children'), | ||
Output('skeleton-users', 'visible'), | ||
Output('card-active-users', 'children'), | ||
Output('skeleton-active-users', 'visible'), | ||
Output('card-trips', 'children'), | ||
Output('skeleton-trips', 'visible'), | ||
Output('fig-sign-up-trend', 'figure'), | ||
Output('skeleton-sign-up-trend', 'visible'), | ||
Output('fig-trips-trend', 'figure'), | ||
Output('skeleton-trips-trend', 'visible'), | ||
], | ||
[ | ||
Input('store-uuids', 'data'), | ||
Input('store-trips', 'data'), | ||
Input('date-picker', 'start_date'), | ||
Input('date-picker', 'end_date'), | ||
Input('url', 'pathname'), | ||
Input('home-page-load', 'children') | ||
] | ||
) | ||
def update_card_active_users(store_uuids): | ||
uuid_df = pd.DataFrame(store_uuids.get('data')) | ||
def update_all_components(store_uuids, store_trips, start_date, end_date, pathname, _): |
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.
I am seeing that we used to have multiple, smaller callbacks for each card. Now we have one big callback that handles everything– which is worse from a code tidiness standpoint.
Can you explain the rationale behind this?
Summary
This pull request introduces skeleton loading screens for the components in
home.py
, providing better user feedback during data fetching and updates. The skeleton loading states will appear while waiting for all components to finish loading, ensuring a smooth transition and improved user experience.Changes
Added
dcc.Loading
components with skeleton loading states to:The skeleton loading will remain active until all components are fully loaded, at which point the actual data will be displayed.
Adjusted callbacks to ensure they trigger the skeleton loaders when necessary, providing real-time feedback during data fetching.
Impact
Demo:
Screen.Recording.2024-10-17.at.5.07.46.PM.mov
Resolves #134