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

map size is saved between page changes #792

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trosborn
Copy link
Contributor

This is a fix for issue #612. I couldn't figure out how to write a test for this because AFAIK the button to trigger map size is not loaded in the test environment.

I used session storage which will save the map size within the current tab. If we want it to be persistent, I can easily switch it to local storage.

@anselmbradford
Copy link
Member

@trosborn Thanks so much for tackling another issue! I modified your code a little in https://github.com/codeforamerica/ohana-web-search/tree/612-map-size-setting, let me know what you think? You can push those changes or I can open a PR from that branch. I think it would be preferable to add a "click" method to the MapSizeControl API so that it can be programmatically clicked without referencing the DOM a second time. Then afterward check the map size in the sessionStorage just after the map renders. I lumped this code into _pluginsLoaded to eliminate the method that would be called only once.

This could perhaps be refactored a bit more to set the map size before the map renders, instead of just after, but FYI that I have in mind to refactor the map overall to simplify it so that instead of small/large states it's show/hide states, with the only showing state being the large map. This is based on user research feedback.

@trosborn
Copy link
Contributor Author

Those changes make a lot of sense. Thanks for cleaning up my code! I think it is easiest if you just make a new PR from your modifications.

Since you're planning a refactor of the maps I'll avoid working on anything maps-related for the time being.

@anselmbradford anselmbradford mentioned this pull request Apr 7, 2015
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