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

Dynamic line previews in line preset icons #5888

Merged
merged 9 commits into from
Feb 14, 2019

Conversation

quincylvania
Copy link
Collaborator

@quincylvania quincylvania commented Feb 13, 2019

Closes #5839. Re: #5873, #5882.

  • Remove embedded line geometry from iD line feature icons
  • Add dynamic line previews to all line preset icons
  • Use the same stroke styling for line icons as when rendering the lines in the map
  • Icon styling can be overridden. For example, the stroke/casing of cycle paths is reversed for the light background

This PR does not address embedded geometry in relation icons.

  • The stroke-dasharray could probably be improved for railways, ferry routes

Nicer icons when using line preset as areas:
screen shot 2019-02-13 at 4 17 07 pm

A selection of the new icons:
screen shot 2019-02-14 at 9 59 22 am
screen shot 2019-02-13 at 4 15 01 pm
screen shot 2019-02-13 at 4 14 53 pm
screen shot 2019-02-14 at 9 59 05 am
screen shot 2019-02-13 at 4 14 19 pm
screen shot 2019-02-13 at 4 14 09 pm
screen shot 2019-02-13 at 4 16 16 pm
screen shot 2019-02-13 at 4 15 32 pm
screen shot 2019-02-13 at 4 15 17 pm

@quincylvania quincylvania added the map-renderer An issue with how things are rendered in the map label Feb 13, 2019
@quincylvania quincylvania self-assigned this Feb 13, 2019
@kymckay
Copy link
Collaborator

kymckay commented Feb 13, 2019

It sounds crazy and I've checked your maths (10.5 + 6/2 = 13.5), but I swear the vertices are slightly lower than the line 😆

@quincylvania
Copy link
Collaborator Author

It sounds crazy and I've checked your maths (10.5 + 6/2 = 13.5), but I swear the vertices are slightly lower than the line 😆

@SilentSpike Are you seeing this in the images or live? Now I'm worried that certain browsers or screens don't like the partial-pixel alignment.

@kymckay
Copy link
Collaborator

kymckay commented Feb 13, 2019

@quincylvania I noticed it particularly in the residential road image, then checked a live instance and see the same. Took a screenshot and see a 1px difference

Edit:

Scratch that, I realised that's not a true test since I had zoomed the page there. However, here is a screen shot I just took at default zoom running a live instance where the difference is clearly visible:

image

I checked your images and there actually isn't a difference where I thought I saw one. So it must be monitor specific.

@bhousel
Copy link
Member

bhousel commented Feb 14, 2019

@SilentSpike Are you seeing this in the images or live? Now I'm worried that certain browsers or screens don't like the partial-pixel alignment.

Maybe try -

  • setting an explicit viewBox=0 0 40 20 attribute on the svg. (width and height should do the same thing, but I'm not 100% sure).
  • changing lineSvg's height from 20 to 19 or 21? (you can open Chrome's dev tools and play with this value live)

Retina screens should handle the half pixel alignment ok - non retina screens would blur it. Also SVG strokes are measured from the center of the stroke

OH and I'd draw those vertices as svg circles, not rounded divs. Actually that's probably the real issue and the lines are probably fine.

- Render vertices with SVG
- Add viewBox to SVG
- Don't reload static parts of the SVG on every update
- Use dimensions that are friendly for low-res displays
- Use nicer-looking stroke dash patterns for ferry routes and railways
@quincylvania quincylvania requested a review from bhousel February 14, 2019 15:01
@quincylvania
Copy link
Collaborator Author

@bhousel Thanks for the tips! I implemented them and this should be ready for review.

@SilentSpike I changed the line casing in the icons to use full-pixel increments, which I think was the problem. Let me know if you're still seeing issues. I updated the railways screenshot so you can check that, or preferably on live.

Copy link
Member

@bhousel bhousel left a comment

Choose a reason for hiding this comment

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

This looks great, and you can merge it whenever you're happy with it 😄
Are you going to do the relation ones too?

@quincylvania quincylvania merged commit ea3a85e into master Feb 14, 2019
@quincylvania quincylvania deleted the dynamic-line-icon-lines branch February 14, 2019 17:27
@quincylvania quincylvania added this to the 2.14.0 milestone Feb 14, 2019
@quincylvania
Copy link
Collaborator Author

quincylvania commented Feb 14, 2019

@bhousel Thanks! I could do the relation ones but I'm not sure that would unblock new features like this did for #5873. Feel free to create an issue.

@bhousel
Copy link
Member

bhousel commented Feb 14, 2019

@bhousel Thanks! I could do the relation ones but I'm not sure that would unblock new features like this did for #5873. Feel free to create an issue.

It seems kind of silly not to just do all of them at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map-renderer An issue with how things are rendered in the map
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants