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 identity function example #4057

Merged
merged 11 commits into from
Jan 30, 2017
Merged

Add identity function example #4057

merged 11 commits into from
Jan 30, 2017

Conversation

sarahkleins
Copy link
Contributor

@sarahkleins sarahkleins commented Jan 27, 2017

Launch Checklist

  • Briefly describe the changes in this PR
  • Code review @lucaswoj @mollymerp
  • Copy and style review

This PR adds an example using the identity function for data-driven styling, responding to this issue.

screenshot 2017-01-27 12 32 36

---
layout: example
category: example
title: Style lines using data-driven styling and the identity property function.
Copy link
Contributor

Choose a reason for hiding this comment

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

All that's visible of this title in the page's fixed-width sidebar is

Style lines using data-driven styling an..

Is there a way we can word the title to capture the intent of the example ("Add identity function example")
visible in the sidebar? Perhaps one of these titles:

Style lines using an identity property function
Style lines using an identity function
Style using an identity function
Style using an identity property function

{
'type': 'Feature',
'properties': {
'color': red,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the string 'red' here 😄

'properties': {
    'color': 'red',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lucaswoj! I added the red and blue variables here to show the output color is controllable (the colors are a little more muted than the 'red' and 'blue' string output). But if you think the string should be here can certainly change.

}
]
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This example would be more readable if we used simpler geometries (shorter lines, fewer lines, or points)

map.addLayer({
'id': 'lines-layer',
'type': 'line',
'source': 'lines-source',
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a standard of using inline sources when there is a 1 source : 1 layer relationship. See #3901 for an example and a little more context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj Sorry 😬 I'm a little unclear on this -- should the source obj be added as the value to the layer's source property?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep instead of calling map.addSource separately, you can declare the source inline in the addLayer function

map.addLayer({
  id: 'lines', // this will be the layer _and_ source id
  type: 'line',
  source: {
    type: 'geojson',
    data: {...}
  },
  paint: {...}
})

'type': 'line',
'source': 'lines-source',
// The identity function does not take a 'stops' property
// Instead, the property value (in this case, 'color') on the datasource will be the direct output.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, the property value (in this case, 'color') on the datasource source will be the direct output.

The word "source" is more idiomatic than "datasource" in the context of Mapbox GL.

'line-width': 3,
'line-color': {
'type': 'identity',
'property': 'color'
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation here looks a little off

@sarahkleins
Copy link
Contributor Author

sarahkleins commented Jan 30, 2017

Good for another round of review here @mollymerp @lucaswoj

@mollymerp
Copy link
Contributor

@sarahkleins the tests are failing on eslint, so once that is fixed (npm run lint -- --fix should get rid of most of them) and all the tests pass go ahead and merge.

@sarahkleins sarahkleins merged commit 3cee599 into mb-pages Jan 30, 2017
@mollymerp mollymerp deleted the identity-function-example branch January 31, 2017 00:03
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.

3 participants