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-in-Map overview box #2554

Merged
merged 8 commits into from Mar 17, 2015
Merged

Map-in-Map overview box #2554

merged 8 commits into from Mar 17, 2015

Conversation

bhousel
Copy link
Member

@bhousel bhousel commented Mar 12, 2015

  • press 'M' to toggle
  • shows current zoom - 6
  • with locator overlay and bounding box

Pretty sweet!

map-in-map3

@aaronlidman

@bhousel bhousel changed the title Map-in-Map overview pane Map-in-Map overview box Mar 12, 2015
@aaronlidman
Copy link
Member

Very cool. @bhousel what is left to do before this is in a state to merge?

Also are all the d3 changes you made in and final? I'm having trouble getting this working locally. This is as far as loading gets me on map-in-map branch and master:

screen shot 2015-03-12 at 10 43 59 am

Cannot read property 'matches' of undefined at https://github.com/openstreetmap/iD/blob/master/js/id/renderer/features.js#L308

@bhousel
Copy link
Member Author

bhousel commented Mar 12, 2015

what is left to do before this is in a state to merge?

I want to have the mini-map capture some mouse events, rather than passing them all through to the main map. Right now you can actually edit stuff underneath the mini-map, which is kind of hack. I'm going to see how complicated it is would be to implement some reasonable zoom/pan behavior.

I guess I can also try to do a nominatum lookup for some "Where am I" text (#2515)

Anything else you can think of?

Also are all the d3 changes you made in and final? I'm having trouble getting this working locally. This is as far as loading gets me on map-in-map branch and master:

Ah, thanks for finding that, it's something I broke in 3308b55, not D3's fault. I'll fix it in master.
(I can see how it could happen if a way has a parent relation that doesn't have any matched features. Like a site relation or something).

@aaronlidman
Copy link
Member

Sounds great, looking forward to it.

bhousel added a commit that referenced this pull request Mar 12, 2015
(e.g. a site relation with a fence in it)
Also, updated the test graph to contain one of these.
see #2554 (comment)
@bhousel
Copy link
Member Author

bhousel commented Mar 14, 2015

This is in pretty good shape now.. Panning the mini-map will recenter the main map, which is nice.

I did experiment with using nominatim to reverse geocode a description of where the map is centered, but I wasn't happy with the results. Most of the time it would return a "hamlet" that is either too obscure or total fiction. I had to go all the way out to the state level just to get it to not do that.

So I think we should just stick to the locator overlay tiles that we're displaying - they are better anyway.

@aaronlidman
Copy link
Member

This works great, perfect.

Should the left margin match the buttons in the top bar?
screen shot 2015-03-15 at 4 29 11 pm

@bhousel This just needs tests and it's good to merge?

@bhousel
Copy link
Member Author

bhousel commented Mar 16, 2015

Should the left margin match the buttons in the top bar?

I guess we could? My thought was actually to just set aside this region where extra UI widgets/plugins could render themselves. Want to loop @samanpwbb in tomorrow and see what he thinks? The Mapillary photos render themselves to the bottom right corner.. Maybe there is a coherent way we could organize these widgety things.

This just needs tests and it's good to merge?

We don't really have test coverage of any of the other UI classes.. I'm not sure how to do it.

Also, I just realized - I think I need to pick a different hotkey. 'M' is used for move too.

@@ -683,7 +683,7 @@ a:hover .icon.out-link { background-position: -500px -14px;}
float: left;
height: 100%;
overflow: hidden;
z-index: 2;
z-index: 10;
Copy link
Member

Choose a reason for hiding this comment

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

This change messes up z-index order:
screen shot 2015-03-15 at 10 17 48 pm

@samanpwbb
Copy link
Member

Since this is a navigational aid, what about adding it as a fourth item in the navigation toolbar below 'show my location':

screen_shot_2015-03-15_at_10_41_14_pm

I'd also suggest moving the mapillary overlay to the top right, with a higher z-index than the minimap, so it occupies the same space as the minimap when active. This will open up more workspace if user is using both minimap and mapillary.

@bhousel
Copy link
Member Author

bhousel commented Mar 16, 2015

I don't have really strong feelings either way, but I was kind of thinking longer term of evolving it into something like this:

plugin all the things

Where plugins might include minimap, Mapillary, distance measurement tool, task manager integration, etc etc. It looks kind of like Photoshop I guess.

Then we wouldn't add any more buttons to iD, except one for installing and managing the user's plugins/widgets/addons. With the goal eventually pulling more of these things out of iD.

(yes I have plugins on the brain, and I'm probably getting ahead of myself)

@samanpwbb
Copy link
Member

Where plugins might include minimap, Mapillary, distance measurement tool, task manager integration, etc etc. It looks kind of like Photoshop I guess.

Then we wouldn't add any more buttons to iD, except one for installing and managing the user's plugins/widgets/addons. With the goal eventually pulling more of these things out of iD.

Moving towards a plugin system will definitely be great for keeping complexity at bay for beginners but enabling features for power users. I am afraid the design sketch you posted would be a too inflexible though. Different plugins will need design solutions that take different forms. I guess we can cross that bridge when we need to.

Anyway, if this work is going to be refactored soon, probably fine to ship the minimap without too much fuss over the specifics until then.

@aaronlidman
Copy link
Member

We don't really have test coverage of any of the other UI classes

Ok, didn't know that.

This works really well, seems solid to me. If you're happy with it I say merge away. 👍 We don't need to worry about other plugins for a while, but good to think about for now.

cc @jfirebaugh

Also, a few optimizations:
* don't redraw the minimap unless mainmap dispatched a full redraw event
* don't recenter mainmap on zoomend unless minimap actually got panned
@bhousel
Copy link
Member Author

bhousel commented Mar 17, 2015

Thanks! @samanpwbb I agree that plugins will be need ability to hook into different parts of iD and have very different UI depending on what they do. Definitely needs more thought.

One final thing I added today was making it so that you can zoom/unzoom with the mousewheel, to control how far zoomed out the minimap is. Very useful.

bhousel added a commit that referenced this pull request Mar 17, 2015
@bhousel bhousel merged commit b6f428a into master Mar 17, 2015
@bhousel bhousel deleted the map-in-map branch March 17, 2015 05:33
paulmach pushed a commit to paulmach/iD that referenced this pull request Jun 8, 2015
(e.g. a site relation with a fence in it)
Also, updated the test graph to contain one of these.
see openstreetmap#2554 (comment)
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.

None yet

3 participants