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

add progress indicator to the result section #706

Merged
merged 4 commits into from Jun 1, 2020

Conversation

Chong-anson
Copy link
Contributor

@Chong-anson Chong-anson commented May 27, 2020

Related issues and PRs

Description

  • create a spinner when the plot is loading. The spinner is aligned with the indicator to the button.

Impacted Areas in the application

  • add PlotSpinner component which renders the spinner logo.
  • add ternary logic to the plot components so that when the result "isRunning", it will render the spinner icon. Otherwise, it will the normal components
  • edit result css file to centre the spinner icon

Testing

@vercel
Copy link

vercel bot commented May 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/covid19-scenarios/covid19-scenarios/3flh3tfrr
✅ Preview: https://covid19-scenarios-git-fork-chong-anson-progress-indicator.covid19-scenarios.now.sh

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #706 into master will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   24.58%   24.53%   -0.06%     
==========================================
  Files         134      135       +1     
  Lines        2896     2902       +6     
  Branches      386      388       +2     
==========================================
  Hits          712      712              
- Misses       2184     2190       +6     
Impacted Files Coverage Δ
src/components/Main/Results/PlotSpinner.tsx 0.00% <0.00%> (ø)
src/components/Main/Results/ResultsCard.tsx 0.00% <0.00%> (ø)
...ponents/Main/Scenario/ConfirmedCasesDataPicker.tsx 0.00% <0.00%> (ø)

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented May 27, 2020

@Chong-anson Thanks, it looks promising, however I insist on this important feature (from the original issue):

  • should not obscure the plot completely, such that the moment when changes arrive can be seen

The whole idea of the app is to allow user to explore different parameters and to see how they affect the results (for example, when relaxing the quarantine measures, user should be able to see that the death plot goes up, etc.)
In this PR, when loading indicator is shown, the plot disappears and the entire results section changes its layout. This makes it difficult to track the changes visually when changing parameters.

Instead of conditional rendering (spinner vs plot) we could implement a semi-transparent overlay on top of the plot, such that the data and the transition from one result to another is visible at all times.

Note, that rendering the whole plot from scratch is very costly (takes from 500 to 1000ms on average machine), so we want to ensure that only the changed items are re-rendered (for example, axes, grid and legend stays the same). This means that we need to avoid unmounting the plot component, changing it's size and changing its props unnecessarily.

Also, currently, the size is changing when switching between the spinner and the plot. Implementing overlay should hopefully fix that. Additional bonus for ensuring that there is no size jump on first render (you know, when the results section "unrolls" down on app load).

This is a creative issue, so feel free to experiment, implement your own idea of how it may look like and refactor the code structure when needed.

@Chong-anson
Copy link
Contributor Author

Thanks for your detailed comment! I will work on that!

@Chong-anson
Copy link
Contributor Author

Chong-anson commented May 30, 2020

Hi Ivan, I edited the components in ResultCard and added some CSS. Sorry for the delay, my internet was down. I tried to work on the "unrolling issue" in the first render, but have been stuck. As the render was handled by React-Resize-Detector, I cannot interfere with the sizing

@codeclimate
Copy link

codeclimate bot commented May 30, 2020

Code Climate has analyzed commit ac025d0 and detected 0 issues on this pull request.

View more on Code Climate.

@ivan-aksamentov
Copy link
Member

This looks good thanks!
Resolves #688

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.

None yet

2 participants