-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add function and interpolation mode to style property docs #7958
Conversation
@boundsj, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @jfirebaugh to be potential reviewers. |
'interpolation mode, an `MGLSourceStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval`, `MGLInterpolationModeCategorical, or `MGLInterpolationModeIdentity` ' + | ||
'interpolation mode, or an `MGLCompositeStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval` or `MGLInterpolationModeCategorical` interpolation mode`'; | ||
} else { | ||
if (property.function == "interpolated") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use the ===
operator unless type coercion is required.
doc += '`\n\nThis property can be set to an `MGLStyleConstantValue` or an `MGLCameraStyleFunction` using an `MGLInterpolationModeExponential` or `MGLInterpolationModeInterval` ' + | ||
'interpolation mode'; | ||
} else { | ||
doc += '`\n\nThis property can be set to an `MGLStyleConstantValue` or an `MGLCameraStyleFunction` using an `MGLInterpolationModeInterval` interpolation mode'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funky indentation.
if (property["property-function"]) { | ||
doc += '`\n\nThis property can be set to an `MGLStyleConstantValue`, an `MGLCameraStyleFunction` using an `MGLInterpolationModeExponential` or `MGLInterpolationModeInterval` ' + | ||
'interpolation mode, an `MGLSourceStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval`, `MGLInterpolationModeCategorical, or `MGLInterpolationModeIdentity` ' + | ||
'interpolation mode, or an `MGLCompositeStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval` or `MGLInterpolationModeCategorical` interpolation mode`'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End this sentence with a period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concatenating strings is leading to long, difficult to follow sentences — could we organize these into tables or otherwise section them?
if (property["property-function"]) { | ||
doc += '`\n\nThis property can be set to an `MGLStyleConstantValue`, an `MGLCameraStyleFunction` using an `MGLInterpolationModeExponential` or `MGLInterpolationModeInterval` ' + | ||
'interpolation mode, an `MGLSourceStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval`, `MGLInterpolationModeCategorical, or `MGLInterpolationModeIdentity` ' + | ||
'interpolation mode, or an `MGLCompositeStyleFunction` using an `MGLInterpolationModeExponential`, `MGLInterpolationModeInterval` or `MGLInterpolationModeCategorical` interpolation mode`.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The end of this string has a stray tick before the period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. At the very least, we should use semicolons to separate list items that contain commas. |
@friedbunny @1ec5 I did not find markup for creating arbitrary tables in jazzy. Are the changes in a749e82 suitable? Example: |
That looks much better, thanks @boundsj. For tables in jazzy, we’ve been resorting to HTML (but that tends to look terrible inline). |
We also have some instances of Markdown tables in the jazzy guides, but we only get away with that because the guides don’t appear in Quick Help or any headers. |
I'm liking the even more bullets look anyway. |
@@ -281,6 +281,30 @@ global.propertyDoc = function (propertyName, property, layerType, kind) { | |||
doc += `\n\nThis attribute corresponds to the <a href="https://www.mapbox.com/mapbox-gl-style-spec/#${anchor}"><code>${property.original}</code></a> layout property in the Mapbox Style Specification.`; | |||
} | |||
} | |||
doc += '\n\nThis property can be set to one of the following values:\n\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’re saying what kind of values the property can be set to, not which values specifically. Also, avoid passive voice:
You can set this property to an instance of:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c602839
doc += '\n\nThis property can be set to one of the following values:\n\n' + | ||
'- `MGLStyleConstantValue`\n'; | ||
if (property["property-function"]) { | ||
doc += '- `MGLCameraStyleFunction` with an interpolation mode of:\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, we’ve been using *
syntax in other places, but I don’t think it matters much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in c602839 for consistency
@@ -9,34 +9,70 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
/** | |||
Controls the scaling behavior of the circle when the map is pitched. | |||
|
|||
This property can be set to one of the following values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t a property, it’s an enumeration value. Enumeration values don’t need this extra text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! Updated in c602839
9ae227e
to
c602839
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Before merging, please make sure the various regular expressions in make darwin-update-examples
(or make style-code
, which depends on it) can cope with the new documentation, just in case.
Fixes #7935
This adds documentation to each style layer property that specifies the function and interpolation mode combinations that will work for the property.
In some cases, function and interpolation stop combination values that don't make sense will be coerced into a values that do. For example, a camera function with and exponential interpolation mode that is set on a property that is a piecewise-constant function will be converted into a camera function with an interval interpolation mode.