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

enable specifying scale/translate in query params for a map #883

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

devvmh
Copy link
Contributor

@devvmh devvmh commented Oct 30, 2016

the issue as framed from the users perspective: https://trello.com/c/J1ryF0gX/49-enable-map-link-to-be-shared-with-the-zoom-and-pan-preset-for-viewer

e.g. http://localhost:3000/maps/12?scale=0.5&translate=-350,300

related to issue #833

The second commit actually updates the query string when you scale or translate the map, via monkey patching the JIT functions. I'm not sure how I feel about this approach. Also, it doesn't seem to be 100% accurate.

@devvmh devvmh added this to the Active (no deadline) milestone Oct 30, 2016
@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch from 113c67e to a43fb2c Compare October 30, 2016 06:02
@@ -125,4 +125,7 @@ describe('Metamaps.Util.js', function () {
describe('resizeCanvas', function () {
it.skip('TODO need a canvas')
})
describe('queryParams', function () {
it.skiy('TODO need window')
})
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be it.skip (spelling)

@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch from 3b39824 to 4568004 Compare October 30, 2016 13:11
@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch from 4568004 to ca86e4e Compare November 29, 2016 16:02
@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch from ca86e4e to 206a327 Compare December 11, 2016 23:01
@devvmh
Copy link
Contributor Author

devvmh commented Dec 12, 2016

I got it to update the query string when you zoom/pan the map in this PR!

@Connoropolous
Copy link
Member

this is exciting! would love to see this one out on heroku for testing soon as well. I've started reading through the code.

@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch 2 times, most recently from be218cf to 1643448 Compare December 18, 2016 21:34
const updateScaleInUrl = debounce(() => {
Util.updateQueryParams({ scale: self.mGraph.canvas.scaleOffsetX.toFixed(2) })
Util.updateQueryParams({ scale: self.mGraph.canvas.scaleOffsetX.toFixed(2) }, pathname)
Copy link
Member

Choose a reason for hiding this comment

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

isn't pathname undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed. it was working anyways since I had a default argument in Util, but fixing is good

@@ -166,7 +167,7 @@ const Visualize = {
const updateTranslateInUrl = debounce(() => {
const newX = self.mGraph.canvas.translateOffsetX.toFixed(2)
const newY = self.mGraph.canvas.translateOffsetY.toFixed(2)
Util.updateQueryParams({ translate: `${newX},${newY}` })
Util.updateQueryParams({ translate: `${newX},${newY}` }, pathname)
Copy link
Member

Choose a reason for hiding this comment

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

and here?

@Connoropolous
Copy link
Member

I wonder about calling these zoom and pan in the url, just for easier readability / understanding by users, less engineer-y

@Bortseb
Copy link
Member

Bortseb commented Jan 15, 2017

FWIW I like zoom more than scale (for similar reasons to connor)... But I prefer translate slightly more than pan because it is a math term, instead of a film (?) term... and we aren't really panning a camera :P

Not a big deal for me either way though

@Connoropolous
Copy link
Member

Connoropolous commented Jan 15, 2017 via email

@Connoropolous Connoropolous force-pushed the feature/pan-zoom-in-url branch from bf42daa to 5bdc792 Compare March 4, 2017 17:07
@Connoropolous
Copy link
Member

@devvmh I'm going to work on this a little bit and come up with a final version of how to store it in the URL. This will be so useful to get in!

@devvmh
Copy link
Contributor Author

devvmh commented Mar 4, 2017

That's great!

This is a big "public api" decision, so i think it's prudent to really think it through before pushing to metamaps.cc. once we enable it it'll be hard to turn off. So I'm okay if we merge, but also ok if we wait

@Connoropolous
Copy link
Member

yeah, exactly. agreed. I want to design a good solution here

@Bortseb
Copy link
Member

Bortseb commented Mar 8, 2017

Can't wait to have this one deployed! It has so many powerful uses and implications

@devvmh devvmh force-pushed the feature/pan-zoom-in-url branch from 5bdc792 to e0eb3e6 Compare September 23, 2017 18:58
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.

3 participants