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

Add symbol-z-order symbol layout property to style spec #7219

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Aug 29, 2018

Launch Checklist

@ryanhamley
Copy link
Contributor Author

See #7076 for background on this PR.

I can think of two alternatives to adding a new style spec property.

  1. We can make this a runtime setting.

    • Downside: I don't think it makes much sense semantically and organizationally since the sorting only applies to symbols, not all layers on a map. Disabling this sorting is closely linked to the existing style spec symbol layout properties *-allow-overlap and *-ignore-placement so it's logical to include this as another style spec property.
  2. We could push forward with adding a mechanism such as some expression syntax to allow for generalized manual sorting of all feature types as discussed in Style spec: Add ability to sort features #4361.

    • Downside: This is significantly more work and possibly overkill for this particular use case.

The downside of this PR's approach obviously is a new property on the style-spec which is something we have to be prepared to maintain for awhile. If/when #4361 is implemented, does this feature become obsolete? I feel like symbol sorting by y is a specialized case that users may want a simple way to disable even if they plan on doing further sorting. And of course this concern assumes we eventually implement general sorting.

Two further thoughts/questions:

  1. Is symbol-sort-by-y an appropriate name or would we prefer something more descriptive like sort-overlapping-symbols (which doesn't specify how the symbols are sorted).

  2. Is the description of the property in v8.json accurate and informative enough for users? And is it true that non-skeumorphic icons won't benefit from symbol sorting and are therefore good candidates for symbol-sort-by-y: false?

Tagging Carto and Studio into this discussion: @nickidlugash @samanpwbb

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Aug 29, 2018

Overlapping icons with default sorting behavior
screen shot 2018-08-29 at 12 59 10 pm

Overlapping icons with symbol-sort-by-y: false (note that because the building icon is the first feature in the source data it is below the other two icons now that sorting by y is disabled)
screen shot 2018-08-29 at 12 59 04 pm

@samanpwbb
Copy link
Contributor

samanpwbb commented Aug 29, 2018

Overall approach looks good to me. I think it's fine to make this a new spec property since it's a straightforward boolean. if/when we take on #4361 we can deprecate or de-emphasize the property.

Is symbol-sort-by-y an appropriate name or would we prefer something more descriptive like sort-overlapping-symbols (which doesn't specify how the symbols are sorted).

What about sort-relative-to-viewport (or sort-by-viewport)? I find sort-by-y a little unclear - I needed to read the description to understand what it meant.

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Aug 30, 2018

Renamed symbol-sort-by-y to more descriptive symbol-sort-relative-to-viewport and edited description of property in docs

@ryanhamley
Copy link
Contributor Author

screen shot 2018-08-31 at 4 38 12 pm

@jfirebaugh
Copy link
Contributor

Let's make this symbol-z-order, an enum-valued property with possible values "viewport-y" and "source". That gives us the flexibility to extend the property with other options. (I can imagine a request for "map-y" for example.)

@samanpwbb
Copy link
Contributor

samanpwbb commented Aug 31, 2018

Let's make this symbol-z-order, an enum-valued property with possible values "viewport-y" and "source". That gives us the flexibility to extend the property with other options. (I can imagine a request for "map-y" for example.)

I like this option 👍. Why viewport-y instead of just viewport though?

@ryanhamley
Copy link
Contributor Author

Yep. Makes sense. I'll change this and the native PR over to symbol-z-order.

@ryanhamley ryanhamley changed the title Add symbol-sort-by-y symbol layout property to style spec Add symbol-z-order symbol layout property to style spec Sep 1, 2018
@ryanhamley ryanhamley requested a review from ChrisLoer September 5, 2018 18:57
Copy link
Contributor

@ChrisLoer ChrisLoer 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 right to me! 🚀

"z-order" threw me off at first because it's not controlling z in the GL coordinate sense, but I came around to it as a pretty natural description of what this does.

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Sep 5, 2018

Ah yeah. I think most front end developers will understand it pretty intuitively as being analogous to the CSS z-index property though.

@ryanhamley ryanhamley merged commit e8fd41a into master Sep 5, 2018
@ryanhamley ryanhamley deleted the sort-by-y branch September 5, 2018 20:00
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.

Style spec: add option to disable y-position sort for symbol features
4 participants