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

Improvements to automatic zooming #49

Merged
merged 5 commits into from
Oct 4, 2015
Merged

Conversation

duncanfwalker
Copy link
Contributor

@foobacca
Copy link
Contributor

foobacca commented Jan 7, 2015

Thanks for this, I've had a quick look at it looks good.

@paulmr @willowo : do you think this is a good change in behaviour? We've had a conversation before about whether the zoom should automatically change when filters change. I quite like the new behaviour this pull request enables (zoom to just view the countries doing the trade) but if you're already zoomed in on the area of interest then it might get annoying.

Maybe we need to get the options area implemented and then this could go there?

@ghost
Copy link

ghost commented Jan 8, 2015

How does it behave on the time slider? I think we'd want to be sure the zoom doesn't change during time-slider runs.

@willowo
Copy link
Member

willowo commented Jan 8, 2015

Fantastic to have fixes to #47 and #48, this will be so useful.

I think it would be great to have the default extent be the extent of the active countries. Obviously this can be over-ridden by then zooming in/out, but when the user first loads the data it gives the instant message of which countries are involved.

My initial reaction would be to maintain the extent when filters are changed (this is useful for quick comparisons e.g. comparing CITES source codes). Currently it is annoying when I have set all my filters and have zoomed into an area of interest, then change a filter and it resets the extent (it seems a minor issue but when trying to make a series of maps for a report it is irritating to spend time zooming in exactly back to the same spot).

What about having a 'lock extent' tick box? A user would zoom in to their preferred extent (or maintain the default), tick the box, then play with the filters/use the time slider and the extent would remain the same. If the box was left un-ticked, every time a filter was changed/time slider was on, the map would zoom to the extent of the active countries. Is that a simple thing? Agree that this tickbox would probably come under Options.

@willowo
Copy link
Member

willowo commented May 19, 2015

@paulmr What do you think about merging this pull request?

@paulmr
Copy link
Contributor

paulmr commented May 19, 2015

Hi @willowo , I haven't had chance to have a detailed look, but at a glance, the code looks great. Have you had a chance to have a play around with it? If not, I have put it here for you:

http://pmr.sdf.org/trademapper/

I think if it seems to work as you want it to, it should be good to merge.

@willowo
Copy link
Member

willowo commented May 19, 2015

Thanks @paulmr I wasn't actually able to see the changes at work before. Thanks once again @duncanfwalker for these suggestions. I think the change to #48 is great and would like to pull that.

The fix for #47 is a bit trickier. My comment above still stands, when trying to make comparisons (e.g. across species), the user needs to keep the extent the same when changing the filters - otherwise it is too hard to compare. I tried the timeslider, and it is a bit disorientating to have it flit all over the globe not to mention difficult to get any sense of how the trade is changing. Would it be possible to have it zoom to the extent of the active countries and just stay there despite changes on the filter or from the timeslider?

@duncanfwalker
Copy link
Contributor Author

Hi @willowo, I should have some time to do at this the week after next. Am I right to say it sounds like just always zooming to the overall extent of all of the the countries in an time series and staying fixed there better fits the need than a 'Lock extent' button?

@willowo
Copy link
Member

willowo commented Jun 22, 2015

Hi @duncanfwalker Yes that is what I am thinking, so that it constantly stays at the same zoom level which will cover all trade data for that .csv regardless of year. Thank you!

A bit of map was showing up behind the test results if you view in the browser
…rdless of year slider position

Only triggers when filter form is changed or CSV is loaded
Looked fine until I saw it online
@duncanfwalker
Copy link
Contributor Author

@foobacca / @paulmr - If this is ok I've got some more stuff (ie fixing the jittery pan behaviour) that I did on this branch so I'll do a PR for that too once this one is in

@duncanfwalker
Copy link
Contributor Author

nudge @foobacca @paulmr @willowo

@willowo
Copy link
Member

willowo commented Sep 15, 2015

@paulmr and @foobacca what do you think of this? Would be great to be rid of the jittery panning! Thanks @duncanfwalker !

@paulmr
Copy link
Contributor

paulmr commented Oct 4, 2015

Great PR, thanks very much for submitting this, and forgive me for taking so long to look at it.

paulmr added a commit that referenced this pull request Oct 4, 2015
Improvements to automatic zooming
@paulmr paulmr merged commit 0c266e7 into trademapper:master Oct 4, 2015
@duncanfwalker
Copy link
Contributor Author

Great- thanks for giving up your Sunday evening to look at it.

I'm not near a computer now but I'll submit the jittery zoom fix some time
this week.
On Oct 4, 2015 9:44 PM, "Paul Roberts" [email protected] wrote:

Great PR, thanks very much for submitting this, and forgive me for taking
so long to look at it.


Reply to this email directly or view it on GitHub
#49 (comment)
.

@willowo
Copy link
Member

willowo commented Oct 4, 2015

Excellent,thanks all!

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.

4 participants