Add data-driven styling #161

pjsier · 2017-10-01T23:40:22Z

I'm working on a project where we've been using Maputnik, and it's been great overall, but we make heavy use of data-driven styles. After looking through issue #96, I took a stab at adding that functionality here. I tried to base the design off of the conversation on that issue, and initially added the default property referenced in #132, but it looks like that may be contingent on updating the style spec.

I don't have a lot of experience with React, so feedback is more than welcome and I'm happy to make any changes

Builds off of the ZoomSpecField component with separate options for handling data-driven properties. Reuses most of the zoom field functionality with tweaks that I tried to keep as small as possible, and the layout is based off of comments on the existing issue.

It looks like default is not supported in this version of the style spec, so pending the PR to update it I'm removing it as an input.

orangemug · 2017-10-04T20:54:59Z

Hey @pjsier thanks for the pull request. I'm a little busy at the moment, but will try and review over the next couple of days. With regards to

...but it looks like that may be contingent on updating the style spec.

If you're working on a project currently, could you give the branch in PR #124 a test, then we can get that merged in as well.

pjsier · 2017-10-04T21:24:38Z

@orangemug thanks for the reply! We've actually been using the updates from that PR (along with these changes) to use the default property on our own instance http://eviction-lab-maputnik.s3-website-us-east-1.amazonaws.com/. We haven't run into any problems, but if there's anything in particular I can take a look at let me know

orangemug · 2017-10-05T05:05:54Z

We've actually been using the updates from that PR

Thats perfect thanks!

orangemug · 2017-10-05T05:11:48Z

@pjsier I took a quick look at http://eviction-lab-maputnik.s3-website-us-east-1.amazonaws.com. First off I think it looks great, congrats this is a big step forwards for maputnik.

I think I've found a bug. I believe the background layer shouldn't support data driven styling, or at least it errors when you try to enable it layers[0].paint.background-color: property functions not supported

orangemug · 2017-10-05T05:14:11Z

@chriswhong, @muesliq would you care to take a look at this PR also?

orangemug · 2017-10-05T05:17:27Z

@pjsier FYI #124 is now merged.

pjsier · 2017-10-05T11:15:00Z

@orangemug great to hear that branch is merged! I pulled it into this PR and re-added the default field. As for the bug, I hadn't been checking for the property-function style spec attribute to decide whether or not to show the data property button, so I added a check for that

orangemug · 2017-10-10T13:29:56Z

Hi @pjsier sorry for taking a fews days to get back to you, I've found another issue

It looks like you're trying to set exponential for fields that don't support exponential for example Text layout properties -> Field try to click the data property button and you get an error layers[84].layout.text-field: exponential functions not supported

Another 'nice to have' modification would be to swap the order of 'zoom function' and 'data function' buttons. A lot more fields have 'zoom function' support and looks a little cleaner in the UI.

Checking the Mapbox style spec properties to see whether or not exponential should be allowed as the property type, defaulting to categorical which appears to work for either type. Also re-orders zoom and data function buttons, aligning zoom right if data not supplied.

pjsier · 2017-10-10T15:35:35Z

@orangemug switching the order of the zoom function and data function buttons was pretty easy, so I updated that. And thanks for catching that bug with the exponential functions. My use case had been mostly around those, and I'd gotten tripped up on the style spec output using interpolated and piecewise-constant instead of exponential and interval.

I updated the component to restrict the available type options based on the function type provided by the spec. Since either type seems to work with categorical, I'm defaulting to that instead of exponential.

I didn't include identity functions because when I tried that out locally it throws errors if it can't immediately parse the supplied value, and since it's based on source data validating that in advance seems like a lot of effort. If it's alright with you, I think leaving them out altogether makes sense, especially since it's the easiest one to enter into the JSON field manually.

orangemug · 2017-10-10T19:01:49Z

Hey thanks @pjsier, that seems to be working now. Another minor issue I guess ZoomSpecField isn't really a good name for this file anymore as it also support data properties.

If you have the time and any ideas of a good name, a rename would be appreciated. Otherwise I think it looks good to merge. The failing tests are just issues with node v4, so I'll ignore them (I might just remove node v4 from the CI in future).

pjsier · 2017-10-10T19:08:42Z

@orangemug great! And I was actually thinking the same thing. Based off of some of the language Mapbox uses in the style spec, maybe FunctionSpecField? It's not a perfect fit, but it would apply to the zoom fields as well and is a bit more general. If that works or you have any other ideas I can make that update

orangemug · 2017-10-10T19:15:07Z

@pjsier FunctionSpecField is better than anything I came up with. Make the change and we'll wait for the tests to pass (except node v4) then I'll merge!

orangemug · 2017-10-11T09:15:09Z

@pjsier ok so I reviewed this pull request again this morning and noticed another issue. Sorry my mistake I'm pretty new to this data function stuff.

The default field seems to be autocompleting to the field name. To see this turn on the data function for a field, focus the 'default field' then lose focus on that field. If you try this on 'Icon layout properties -> Size' the 'default' field' will end up being set to 'icon-size'.

Annoyingly the only way to reset the value is to edit with the JSON, but I think that is to do with our error handling generally rather than an issue with this pull request.

Note: I'll hold off merging #167 until this is merged, so you don't have to deal with the conflicts.

pjsier · 2017-10-11T11:03:57Z

@orangemug ah thanks for catching that. It looks like it was mostly a typo (I was only handling one function parameter but SpecField was returning 2), but I also added a check in to changeDataProperty to allow people to empty the color field

orangemug · 2017-10-11T12:33:08Z

src/components/fields/FunctionSpecField.jsx

+    const dataProps = {
+      label: "Data value",
+      value: dataLevel,
+      onChange: newData => this.changeStop(idx, { zoom: zoomLevel, value: newData }, value)


@pjsier I think you mean zoomLevel here, idx is undefined

@orangemug actually, I think I was accidentally doing some fancy scoping, because it does need to access the idx of the parent to change the stop. About to update that by removing the getDataInput and moving that conditional into the loop so it's more explicit

orangemug · 2017-10-11T12:47:42Z

@pjsier I've added a comment, in the code. Also noticed that some of the properties are throwing errors, updating mapbox-gl^0.40.1 and @mapbox/mapbox-gl-style-spec^9.0.1 fixes those I believe.

We've now got a new error, modifying data function values causes the error array expected, undefined found

pjsier · 2017-10-11T13:05:05Z

@orangemug I updated mapbox-gl and the style spec and clarified that scope, but I'm not seeing the array expected error that you mentioned. Could you give an example of a layer it's happening on?

orangemug · 2017-10-11T15:54:36Z

@pjsier I can't seem to reproduce the issue now. Ok this looks great, there are a couple of UI things I think we could improve

Labels on the 3 text boxes, it's not clear what to enter where at the moment
If you forget to enter a property it'll error in odd ways. Maybe we should disable the stops until the property is selected.
Some UI tests. We need to discuss testing in general the project is pretty untested at the moment

However we can pick those things up later on in new tickets.

Congrats on the feature, this is a big step forward!

orangemug · 2017-10-11T15:56:53Z

I'm working on a project where we've been using Maputnik, and it's been great overall, but we make heavy use of data-driven styles.

Oh also @pjsier it'd be great to hear about what you're working on over in issue #164

pjsier added 3 commits October 1, 2017 18:20

Fix scss linter errors

d731fb2

Remove default from data properties

9e52b0b

It looks like default is not supported in this version of the style spec, so pending the PR to update it I'm removing it as an input.

orangemug mentioned this pull request Oct 4, 2017

Per branch builds #123

Closed

pjsier added 3 commits October 5, 2017 05:42

Merge branch 'master' into feature/data-driven-styles

bba7aa3

Re-add default field after style update

2ffb3e7

Check for property-function support on data styles

1d29f67

Rename ZoomSpecField to FunctionSpecField

4af7a71

orangemug self-requested a review October 11, 2017 10:01

orangemug mentioned this pull request Oct 11, 2017

Fix zoom field order issue #167

Closed

Fix default field bug

b429202

orangemug reviewed Oct 11, 2017

View reviewed changes

Update mapbox deps, clarify data prop scope

fa0067c

orangemug approved these changes Oct 11, 2017

View reviewed changes

orangemug merged commit aa288a1 into maplibre:master Oct 11, 2017

This was referenced Oct 11, 2017

Default property #132

Closed

Is it possible to do data-driven styling? #96

Closed

pjsier mentioned this pull request Oct 16, 2017

Assigning default function type from spec #178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data-driven styling #161

Add data-driven styling #161

pjsier commented Oct 1, 2017 •

edited

Loading

orangemug commented Oct 4, 2017

pjsier commented Oct 4, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

pjsier commented Oct 5, 2017

orangemug commented Oct 10, 2017

pjsier commented Oct 10, 2017 •

edited

Loading

orangemug commented Oct 10, 2017

pjsier commented Oct 10, 2017

orangemug commented Oct 10, 2017

orangemug commented Oct 11, 2017

pjsier commented Oct 11, 2017

orangemug Oct 11, 2017 •

edited

Loading

pjsier Oct 11, 2017

orangemug commented Oct 11, 2017

pjsier commented Oct 11, 2017

orangemug commented Oct 11, 2017

orangemug commented Oct 11, 2017

Add data-driven styling #161

Add data-driven styling #161

Conversation

pjsier commented Oct 1, 2017 • edited Loading

orangemug commented Oct 4, 2017

pjsier commented Oct 4, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

orangemug commented Oct 5, 2017

pjsier commented Oct 5, 2017

orangemug commented Oct 10, 2017

pjsier commented Oct 10, 2017 • edited Loading

orangemug commented Oct 10, 2017

pjsier commented Oct 10, 2017

orangemug commented Oct 10, 2017

orangemug commented Oct 11, 2017

pjsier commented Oct 11, 2017

orangemug Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

pjsier Oct 11, 2017

Choose a reason for hiding this comment

orangemug commented Oct 11, 2017

pjsier commented Oct 11, 2017

orangemug commented Oct 11, 2017

orangemug commented Oct 11, 2017

pjsier commented Oct 1, 2017 •

edited

Loading

pjsier commented Oct 10, 2017 •

edited

Loading

orangemug Oct 11, 2017 •

edited

Loading