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

Preserve modified date range while DetailView is displayed. #641

Merged
merged 8 commits into from
Nov 18, 2020

Conversation

dlaliberte
Copy link
Contributor

closes #623

Prerequisites:

  • Unless it is a hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

Preserve the modified date range in the DetailView between changes of sensor or regions. Also entering and exiting compare mode leave the date range as it is. Only closing the DetailView restores the default, next time it is opened.

@dlaliberte dlaliberte changed the title Dlaliberte/date range2 Preserve modified date range while DetailView is displayed. Nov 17, 2020
@dlaliberte dlaliberte requested a review from sgratzl November 17, 2020 22:17
@netlify
Copy link

netlify bot commented Nov 17, 2020

Preview link ready!

Built with commit ce86141

https://deploy-preview-641--cmu-delphi-covidcast.netlify.app

@netlify
Copy link

netlify bot commented Nov 17, 2020

Preview link ready!

Built with commit 8df489b

https://deploy-preview-641--cmu-delphi-covidcast.netlify.app

@dlaliberte
Copy link
Contributor Author

It actually fails to preserve the date range the first time after opening the DetailView. And if you select a sensor with no data, it loses the date range entirely.

@dlaliberte
Copy link
Contributor Author

Fixed the "no data" situation. The failure to preserve the date on the first time doesn't happen if I am watching with the debugger, so there is a timing issue or something that I am not understanding.

@dlaliberte
Copy link
Contributor Author

Redefined the dateRange variable to use writable, but it still fails (most of the time) the on the first change, but works thereafter. I'm stumped.

Copy link
Member

@sgratzl sgratzl left a comment

Choose a reason for hiding this comment

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

Redefined the dateRange variable to use writable, but it still fails (most of the time) the on the first change, but works thereafter. I'm stumped.

how can I reproduce that? can you make a screenshot showing the behavior?

src/components/DetailView/DetailView.svelte Outdated Show resolved Hide resolved
src/components/DetailView/DetailView.svelte Outdated Show resolved Hide resolved
@dlaliberte
Copy link
Contributor Author

how can I reproduce that? can you make a screenshot showing the behavior?

I'm not sure if I can do a screen recording. Need a tool allowed by Google that works with macbook and/or chrome.

But reproducing this is straightforward. On a fresh page (I click on "COVIDcast" link in header),

  • starting with Map Overview,
  • click on magnifier icon for COVID-Related Doctor Visits,
  • drag the date range slider back a couple months,
  • click the magnifier for COVID-like Symptoms
  • Notice that the date range slider is reset to the small multiple default.
  • Try it again (without restarting), and notice it works correctly thereafter.

Actually, the latest version preserves the change after closing the detail view as well, or going to any other view. That's because it is storing it in detailViewTimeSpan, so it never gets reset to the default after that. It only fails the very first time.

@dlaliberte
Copy link
Contributor Author

Actually, the latest version preserves the change after closing the detail view as well, or going to any other view. That's because it is storing it in detailViewTimeSpan, so it never gets reset to the default after that. It only fails the very first time.

After merging in the latest from origin/dev, now it does work the first time as well. So something has changed maybe about the timing of page loading or whatever, so I wonder if this will be reliable.

Also, the persistence of the date range change could be a useful feature, but now we have the opposite problem, that there is no way to reset to the default, other than refreshing the page. I'll remove it from the store, and see if that works correctly now.

@dlaliberte dlaliberte requested a review from sgratzl November 18, 2020 16:42
Copy link
Member

@sgratzl sgratzl left a comment

Choose a reason for hiding this comment

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

looks good beside the one comment regarding the store

src/stores/index.js Outdated Show resolved Hide resolved
@dlaliberte dlaliberte merged commit fe0a26e into dev Nov 18, 2020
@dlaliberte dlaliberte deleted the dlaliberte/date_range2 branch November 18, 2020 17:40
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.

2 participants