-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios] Reorder classes in jazzy docs sidebar #7338
Conversation
ericrwolfe
commented
Dec 7, 2016
- Move guides front-and-center (to be expanded out to include more guides)
- Organize shape classes heirarchically for clarity
- Clarify features, sources, and layers as related to map styling (as opposed to annotations)
@ericrwolfe, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @boundsj to be potential reviewers. |
@@ -80,15 +91,12 @@ custom_categories: | |||
- MGLTilePyramidOfflineRegion | |||
- name: Geometry | |||
children: |
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.
Let’s also add MGLCoordinateBoundsIntersectsCoordinateBounds
to Geometry. (It’s in Other Functions, currently.)
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.
Did this in 4057ac8.
@@ -23,53 +26,61 @@ custom_categories: | |||
- MGLMapViewDelegate | |||
- MGLStyle | |||
- MGLUserTrackingMode | |||
- name: Annotations | |||
- name: User Interaction |
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.
Now that this section only contains callout views, I think we should move it farther down the list — perhaps after the new Annotations section?
Any ideas of what, if anything, to do with |
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.
Lots of interesting ideas here.
Would you mind keeping platform/macos/jazzy.yml in more or less in sync with this file once you’ve settled on a structure you like? That should be interesting, because some of the classes present on iOS don’t exist yet on macOS or never will, like MGLCalloutView and MGLUserLocationAnnotationView. (If not, that’s fine; I can follow up with an update to that file.)
- MGLGeoJSONSource | ||
- MGLRasterSource | ||
- MGLVectorSource | ||
- name: Style Layers (Abstract) |
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.
I wonder whether dividing classes by abstractness is really a good idea for the style layer classes, which were designed to be a nested inheritance tree, with each level sort of peeling a layer off the onion. On the one hand, categorizing them this way helps developers find the classes they can safely initialize. On the other hand, it also obscures the relationships between each concrete class and the abstract superclasses that contribute to its properties and initializers.
To be sure, one alphabetical list wasn’t especially enlightening, either, but what about a flattened inheritance hierarchy like this:
- MGLStyleLayer
- MGLBackgroundStyleLayer
- MGLForegroundStyleLayer
- MGLRasterStyleLayer
- MGLVectorStyleLayer
- MGLCircleStyleLayer
- MGLFillStyleLayer
- MGLFillExtrusionStyleLayer would go here
- MGLLineStyleLayer
- MGLSymbolStyleLayer
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.
Similarly, a flattened inheritance hierarchy for content sources would look like:
- MGLSource
- MGLShapeSource
- MGLTileSource
- MGLRasterSource
- MGLVectorSource
Once #6940 lands in a future release, the list would look something like this:
- MGLSource
- MGLAbstractShapeSource
- MGLComputedShapeSource
- MGLShapeSource
- MGLTileSource
- MGLRasterSource
- MGLVectorSource
- MGLCalloutView | ||
- MGLCalloutViewDelegate | ||
- name: Protocols and Abstract Classes | ||
children: | ||
- MGLAnnotation |
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 protocol is unlike the others in this category in that the developer can actually make any class they want conform to it, and we’ll treat that class just like an MGLPointAnnotation. I think it should go in the “Annotations” category alongside MGLAnnotationImage and MGLAnnotationView.
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 myriad annotation classes and protocols are confusing, and this PR goes some ways towards improving that situation. Isolating the most commonly used, most functional annotations classes together in the “Annotations" category is a good move.
From a comprehensibility standpoint, I think that MGLAnnotation is where it belongs.
- MGLShapeCollectionFeature | ||
- name: Style Sources |
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.
I think we should avoid saying “style source”, even if a source is part of a style, because it seems to imply that it defines an appearance rather than unstyled content. Since “data source” has a specific meaning on iOS, how about “Map Sources” or “Content Sources”?
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.
Main reasoning here was to communicate that the source classes are associated with runtime map styling in the same way as the feature and style layer classes. Trying to avoid confusion by grouping and differentiating all of these classes from the more primitive shape classes.
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.
I agree with your reasoning, though I can’t shake the feeling that developers will overlook these classes when they want to add content to the map, especially in the case of MGLRasterSource, which isn’t something that people normally think they need to style.
“Content source” is one step away from “data source”, which is a pattern a lot of developers are familiar with due to UITableViewDataSource and similar classes. We can’t really call these classes “data sources”, because that implies a delegate-like pattern and protocols.
@ericrwolfe I pushed some small changes, mainly addressing the review comments I made — feel free to revert if I overstepped. |
children: | ||
- MGLCalloutView | ||
- MGLCalloutViewDelegate | ||
- name: Style Features |
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.
By the way, these classes are also used as feature querying results. Whether a developer uses them more for runtime styling or feature querying probably depends on the use case.
- MGLShapeCollectionFeature | ||
- name: Style Sources |
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.
I agree with your reasoning, though I can’t shake the feeling that developers will overlook these classes when they want to add content to the map, especially in the case of MGLRasterSource, which isn’t something that people normally think they need to style.
“Content source” is one step away from “data source”, which is a pattern a lot of developers are familiar with due to UITableViewDataSource and similar classes. We can’t really call these classes “data sources”, because that implies a delegate-like pattern and protocols.
- MGLTileSet | ||
- MGLVectorSource | ||
- MGLStyleValue | ||
- NSValue(MGLAdditions) |
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.
Style value–related methods are only part of this category; there are also methods related to offline maps.
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.
Hmm, true, guess I'll just exclude then and add a note to MGLStyleValue
b470c91
to
dfde752
Compare
@1ec5 @ericrwolfe This seems ready (enough) to merge, yes? With all of the other jazzy PRs still moving, there will be plenty of opportunity to address other concerns as we break-in these changes. |
- MGLTileCoordinateSystem | ||
- MGLTileSource | ||
- MGLVectorSource | ||
- MGLStyleValue |
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.
What about MGLConstantStyleValue or MGLStyleFunction? MGLStyleValue is a class cluster that includes those two classes, so the developer could get all the information they need with just MGLStyleValue, but it seems odd to have a category with only one item. If the idea is to deemphasize MGLConstantStyleValue and MGLStyleFunction in favor of MGLStyleValue, perhaps we could move this class up to “Maps” alongside MGLStyle.
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.
Yup, idea was to deemphasize MGLConstantStyleValue + MGLStyleFunction. I think it should be grouped relatively close to the style layers, because that's where you're most likely to use them, the top level Map group is too far removed.
On that note, how would you feel about moving MGLStyle down into a new group above the style primitives?
- User Interaction
- Styling the Map
- MGLStyle
- MGLStyleValue
- Style Primitives
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.
That would be fine. I think we’re getting to the point, though, where nearly all the categories are about styling. Time to address realm/jazzy#624, I suppose.
Aside from the one comment above, I think this PR is ready to merge. However, I still have strong reservations about separating the abstract classes from their concrete subclasses in light of realm/jazzy#706..
@@ -23,55 +26,62 @@ custom_categories: | |||
- MGLMapViewDelegate | |||
- MGLStyle | |||
- MGLUserTrackingMode | |||
- name: Annotations | |||
- name: Protocols and Abstract Classes |
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.
I wonder if this category’s title still makes sense given that some abstract classes are in more specific categories below.
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.
Ah yes, I had moved some of those out of their Abstract header I'd seen #7478, which is an improvement (scss maintainability another thing :p), and had forgotten to change them back.
I don't have strong opinions about where these abstract classes should live, so happy to move them with their related siblings and we can circle back on how to group them once the jazzy PR is merged.
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.
OK, let’s move them back then. I’ll also work on a way for a way to provide more info about each class in the table of contents within jazzy.yml so we don’t have to hard-code that list inside SCSS. 😬
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 addressing my concerns about separating the protocols and abstract classes. Feel free to merge this PR as is.
Note to self: these changes also need to be ported to platform/macos/jazzy.yml.
* Move guides front-and-center (to be expanded) * Organize shape classes heirarchically for clarity * Clarify features, sources, and layers as related to map styling (as opposed to annotations)
e016e08
to
63a85c4
Compare
I brought the macOS table of contents more in line with the iOS table of contents in 63a85c4. I had to elide a couple categories because they had too few classes once iOS-only classes were removed. |