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

Don't render one-way arrows on highway features that aren't rendered #925

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

dschep
Copy link
Contributor

@dschep dschep commented Sep 17, 2023

I noticed that the style is rendering oneway arrows on paths. This PR removes those (and on tracks, as they aren't rendered either)

on main:
image

on this branch:
image

@github-actions
Copy link

PR Preview:

Sprite Sheets:

Sprites 1x
Sprites 2x

@ZeLonewolf ZeLonewolf mentioned this pull request Sep 18, 2023
11 tasks
Copy link
Member

@1ec5 1ec5 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 good to me. Let me know if you agree with the suggested changes; I think it’s fine to merge regardless.

Comment on lines 12 to +13
["!", ["in", ["get", "brunnel"], ["literal", ["bridge", "tunnel"]]]],
["!", ["in", ["get", "class"], ["literal", ["path", "track"]]]],
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I find this somewhat more readable, though I don’t think it would affect performance:

Suggested change
["!", ["in", ["get", "brunnel"], ["literal", ["bridge", "tunnel"]]]],
["!", ["in", ["get", "class"], ["literal", ["path", "track"]]]],
["match", ["get", "brunnel"], ["bridge", "tunnel"], false, true],
["match", ["get", "class"], ["path", "track"], false, true],

@1ec5 1ec5 added bug Something isn't working highway-lines labels Sep 18, 2023
@dschep
Copy link
Contributor Author

dschep commented Sep 19, 2023

I felt that using the same type of expression as directly above (for brunnel) was more readable.

@ZeLonewolf
Copy link
Member

I think this version is fine, especially since we'll be presumably throwing it away once tracks and paths actually get implemented anyways. Based on @adamfranco's comments on #717, we're not concerned about a conflict with ongoing work. Therefore, I agree that this is good to merge. Thanks for the PR!

@ZeLonewolf ZeLonewolf merged commit de40a62 into osm-americana:main Sep 19, 2023
@dschep
Copy link
Contributor Author

dschep commented Sep 19, 2023

Yep! I'm also hoping this is temporary, seeing as I ❤️ trails

@dschep dschep deleted the no-path-track-oneway-arrows branch September 20, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working highway-lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants