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

Don't ignore the zoom slider #128

Open
stuartlangridge opened this issue Jan 26, 2020 · 3 comments
Open

Don't ignore the zoom slider #128

stuartlangridge opened this issue Jan 26, 2020 · 3 comments

Comments

@stuartlangridge
Copy link
Collaborator

The slider under a graph alters the time period which the graph covers. However, if the graph gets redrawn (by choosing Weekly or Monthly, or making the graph full-size) then it ignores the current zoom slider setting and just draws it at its default time period, but leaves the slider where it is.

This should abide by the current setting of the slider and draw the graph with that zoom.

@stuartlangridge
Copy link
Collaborator Author

OK. We currently explicitly and deliberately ignore the zoom slider: in particular, we remember separate settings for the zoom slider for each type of graph. (So if you set the slider to something for the Monthly graph, then switch to Weekly and set the slider to something different, then switch back to Monthly, we should reset the slider to the thing you picked for Monthly. See rememberedSliderValues in https://github.com/MeasureOSS/Measure/blob/master/assets/js/chartjs-plugin-adjustable.js#L29.)

However, this feat of memory is not saved anywhere, such as across a page reload. We presumably did this because it seemed like a good idea at the time, although it's possible that now what we think is a sensible thing to do has changed and the slider should carry on across different graph types (and fullscreen graphs): that is, the new proposal is that moving the slider sets a time period to show in the graph (say, from October 2019 to today) and that time period should be persisted across all graphs. But I'm loath to change something where we made a deliberate decision to do it another way without checking why we did that, so @jeremy-lq do you think it is correct to change this so that the time period is what the slider sets, and that time period is what is maintained across different examples of the same graph (so setting the slider to Oct19-today makes that the period shown on both the weekly and the monthly graphs in this widget)?

@jeremy-lq
Copy link
Collaborator

I think setting the slider to Oct19-today should definitely persist from graph -> fullscreen graph. I don't think the current behavior was deliberate, so also keeping it when switching from Monthly to Weekly seems to more intuitive behavior.

stuartlangridge added a commit that referenced this issue Jan 28, 2020
Changing the zoom slider on a graph now attempts to set the graph to show a particular time period.
Switching graph type (Monthly to Weekly, for example) will attempt to, roughly, maintain that time period.
(It's not exact, but it's approximately correct.)
Showing a graph in fullscreen will also now respect both your current setting for the graph type (monthly/weekly) and also the current time period on the zoom slider.
This is issue #128.
@stuartlangridge
Copy link
Collaborator Author

PR for testing.

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

No branches or pull requests

2 participants