Skip to content

add loading indicators#586

Closed
jwildfire wants to merge 2 commits intodevfrom
spinspin
Closed

add loading indicators#586
jwildfire wants to merge 2 commits intodevfrom
spinspin

Conversation

@jwildfire
Copy link
Contributor

Overview

Added 2 types of loading indicators using shinybusy:

  • Added a basic loading indicator that runs whenever server runs for more than 1 second (like when rendering a big chart)
  • Added a modal indicator that pops up while html reports are being generated. This prevents other activity in the app, which wouldn't work anyway since the server is busy rendering the report.

Fix #576. Fix #519.

Test notes

Make sure the new loading indicators are working as expected and don't have any major unintended consequences ...

@jwildfire jwildfire self-assigned this Jun 17, 2021
Copy link
Contributor

@samussiah samussiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spinner locks the app when export fails.

params = params, ## pass in params
envir = new.env(parent = globalenv()) ## eval in child of global env
)
remove_modal_spinner() # remove it when done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line doesn't run if report fails - Demographics Table, Shift Table - and spinner just spins.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just drop these two modules for this release? They don't work on my workstation.

Copy link
Contributor Author

@jwildfire jwildfire Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Shift Table is no longer in the default build after SafetyGraphics/safetyCharts#67, demographics table is exporting for me, but it's just printing the R object (screenshot below). The download RTF button on the chart page is the real "export" workflow for now. So, I think we should just set the export flag to false for it for now. Filed SafetyGraphics/safetyCharts#70 to deal with that in safetyCharts.

We should probably discuss that workflow a bit more with @elimillera at some point, but I think I'm ok waiting until after 2.0

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line doesn't run if report fails - Demographics Table, Shift Table - and spinner just spins.

Yeah, I'm not sure the best workflow here. I guess at the very least we should have some trycatch() logic that throws a decent message to the modal if the export fails.

I think I'm going to leave #517 for a different PR that deals with some of the other issues in the export. I'll close this one, and make a new PR that just resolves #576.

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.

Show loading indicator when initializing charts Add loading indicator on export tab

2 participants