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 data-driven values for text-font #5698

Merged
merged 2 commits into from
Nov 22, 2017
Merged

Enable data-driven values for text-font #5698

merged 2 commits into from
Nov 22, 2017

Conversation

jfirebaugh
Copy link
Contributor

Enables support for data-driven text-font values. Fixes #5568.

The port to native will need to take into account mapbox/mapbox-gl-native#9939 -- my proposed resolution is described in mapbox/mapbox-gl-native#9939 (comment).

@anandthakker
Copy link
Contributor

anandthakker commented Nov 17, 2017

The port to native will need to take into account mapbox/mapbox-gl-native#9939 -- my proposed resolution is described in mapbox/mapbox-gl-native#9939 (comment).

@jfirebaugh I was thinking we would impose this restriction at the level of spec validation (and so in GL JS as well), not just in -native.

@jfirebaugh
Copy link
Contributor Author

Oh, yeah, that's probably a good idea.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

Nice.

We could at some point elaborate possibleValues() for at and var, but 👍 for keeping it conservative for now.

"layout": {
"text-font": ["let", "p", ["array", "string", ["get", "text-font"]], ["var", "p"]],
"text-field": "{foo}"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: let's include "text-font" in the id for these layers, since that's what they're testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this is necessary given that text-font is in the fixture file name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Whoops -- I'd assumed without looking that this was in the functions fixture. 👍 as is

@jfirebaugh
Copy link
Contributor Author

Hmm, validation also needs to prohibit identity functions.

@anandthakker
Copy link
Contributor

Oof, I always forget about those. Another option would be to require data-driven values to be expressions (not functions)

@jfirebaugh
Copy link
Contributor Author

I thought about that, but I think it might be confusing for a long-existing property to have that restriction. We did do it for heatmap-color, but that was (essentially) a new property, and it has a special-case input.

@anandthakker
Copy link
Contributor

I think it might be confusing for a long-existing property to have that restriction.

Hm, yeah good point -- I suppose if someone was already using a zoom-only stop function, they'd expect to be able to upgrade it to a zoom-and-property stop function after this.

@nickidlugash
Copy link

@jfirebaugh looks great – thanks for pushing on this. Can't wait to test out some expression values for text-font! /cc @aparlato

@jplante
Copy link

jplante commented Feb 6, 2018

@jfirebaugh
Just verifying:

  1. This being data-driven does not appear to be updated on the API documentation Style Specification. text-font is listed as "data-driven styling" and "Not yet supported" across platforms.

  2. Even if it does work, does it support identity functions?

@anandthakker
Copy link
Contributor

@jplante Yes, the API docs need to be updated -- thanks for noting that!

does it support identity functions

No, identity functions aren't supported for text-font. (And, to generalize this, text-font expressions must contain all possible output values as literals within the expression)

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.

4 participants