Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.

[WIP] update iOS SDK guides#8354

Merged
jmkiley merged 22 commits into
release-ios-v3.5.0-android-v5.0.0from
jmkiley-update-guides
Mar 17, 2017
Merged

[WIP] update iOS SDK guides#8354
jmkiley merged 22 commits into
release-ios-v3.5.0-android-v5.0.0from
jmkiley-update-guides

Conversation

@jmkiley
Copy link
Copy Markdown
Contributor

@jmkiley jmkiley commented Mar 10, 2017

  • Update the For Style Authors guide to include DDS related terminology
  • Add a data-driven styling guide.
    - [ ] Update Runtime Styling guide as necessary

The data-driven styling guide currently includes more code than the other previous guides, and may be more tutorial. It is intended to illustrate the differences between the different interpolation modes and how to use them, but it can be trimmed down.

Intended to partially address #8192 and the last two items on #7924.

@jmkiley jmkiley added documentation iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 10, 2017
@jmkiley jmkiley added this to the ios-v3.5.0 milestone Mar 10, 2017
@1ec5 1ec5 changed the title [WIP] update iOS SDK [WIP] update iOS SDK guides Mar 10, 2017
@1ec5 1ec5 added the macOS Mapbox Maps SDK for macOS label Mar 10, 2017
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting style functions is a subtask of configuring the map content’s appearance. I think the table mapping function types to MGLStyleFunction subclasses and MGLInterpolationMode values should be moved down here, to help progress the reader from simpler concepts to more advanced concepts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree. Especially since the tables currently repeat for each property.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let’s split up this table into two: one for function types and the other for interpolation modes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is also a terminological difference. It’s worth mentioning that a “function type” is an “interpolation mode” up in the first table.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Function types (“zoom”, “property”, “zoom-and-property”) don’t appear in the style JSON, only in the style specification.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could make this guide platform-agnostic, either by replacing UIColor with some other type like NSNumber that also exists on macOS, or by relying on type inference in Swift and the MGLColor macro in Objective-C. Then we could move this guide to platform/darwin/docs/guides/.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MGLColor is a fine internal workaround, but I don’t believe we’ve encouraged its use publicly(?). Telling developers to use MGLColor has a couple issues:

  1. It’s not necessary for the vast majority of our developers (who are iOS-only).
  2. MGLColor’s purpose and action is opaque — actually using it has no technical downside, but the cognitive burden of learning what it is isn’t negligible. I’d expect a significant portion of our users to just assume that it’s the required way to define colors and not understand why that’s not the case.

At this point, I think it’s more reasonable to expect macOS developers to know UIColor doesn’t exist on that platform, rather than asking iOS developers to use a macro they don’t need.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Definitely onboard with consistently using type inference in Swift, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Point taken about MGLColor being pedagogically problematic. In the Mapbox API Documentation examples in Objective-C, I took to using conditional compilation macros, which are better documented but even more poorly understood by less experienced developers.

What about bringing this guide under the same codegen mechanism that the “Information for Style Authors” guide uses?

  1. Move this guide to platform/darwin/docs/guides/Data-Driven Styling.md.ejs.
  2. Define a cocoaPrefix variable at the top.
  3. Put a <% cocoaPrefix %> variable anywhere we’re currently saying UI.
  4. Extend generate-style-code.js to also write out iOS- and macOS-specific versions of the guide.
  5. Run make style-code.

With this approach, the style-related guides would be more consistent with each other even at the code level. Additionally, it’ll give us the ability to put some of these (rather substantial) code examples in unit tests and insert them into this document automatically, ensuring that the examples stay current. We already use that approach for examples embedded in API documentation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the DDS guide currently lives in platform/ios/docs/guides/, this link is broken in the macOS generated documentation. https://github.com/mapbox/mapbox-gl-native/pull/8354/files#r105503718 would be preferable, but if we can’t do that, then we should conditionalize this sentence on the platform. There are some examples above of only including a sentence on iOS or only on macOS.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MGLColor is a fine internal workaround, but I don’t believe we’ve encouraged its use publicly(?). Telling developers to use MGLColor has a couple issues:

  1. It’s not necessary for the vast majority of our developers (who are iOS-only).
  2. MGLColor’s purpose and action is opaque — actually using it has no technical downside, but the cognitive burden of learning what it is isn’t negligible. I’d expect a significant portion of our users to just assume that it’s the required way to define colors and not understand why that’s not the case.

At this point, I think it’s more reasonable to expect macOS developers to know UIColor doesn’t exist on that platform, rather than asking iOS developers to use a macro they don’t need.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use smart quotes: Mapbox'sMapbox’s

Copy link
Copy Markdown
Contributor

@friedbunny friedbunny Mar 10, 2017

Choose a reason for hiding this comment

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

to illustrate to style a map

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

referred to what is now a MGLCameraStyleFunction.

This seems like unnecessary repetition. Could we say something like: “In iOS SDK v3.4, this function was called MGLStyleFunction.”?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let’s convert -> to (or some other arrow symbol).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or “to”, since this is prose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Definitely onboard with consistently using type inference in Swift, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we create ranges where earthquakes with a magnitude of 0 to just less than 2.5 would be yellow,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can’t rely on GitHub to host these assets — be sure to commit and use the files required for this guide.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely! In the process of doing that. I copied this from a gist and am still working on optimizing the screenshots

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the colon in this heading.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Colon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or “to”, since this is prose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Specifying swift here would get us syntax coloring.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

URL(string:) only returns nil if the string is a malformed URL. Since the URL is formed from a string literal, we know it’s well-formed, so we can ! unwrap the optional without indenting everything below.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But if we were to indent, we should use four spaces for each level of indentation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reminds me: we should find a good place to mention the ability to use #8025, even if it isn’t important enough to show in a code example.

Copy link
Copy Markdown
Contributor Author

@jmkiley jmkiley Mar 11, 2017

Choose a reason for hiding this comment

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

Oh, good point. I will see if I can adjust the example to show that. Alternatively, we can use it in an upcoming example. Or both.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per #8025 (comment), let’s take this opportunity to point out that not every interpolation mode is available for every attribute. The developer should consult the documentation for an individual layout or paint attribute to see which function types and interpolation modes it supports.

@jmkiley jmkiley force-pushed the jmkiley-update-guides branch from 3a5e79a to f9f084a Compare March 12, 2017 21:32
Copy link
Copy Markdown
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

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

Don't forget to update the jazzy.yml files so the new guide shows up in the generated documentation side bar for iOS and macOS.

Also, there are changes to the For Style Author.md.ejs template here that have not yet been picked up by make style-code and committed as part of this PR. I looks like we plan to create a template for the DDS guide (per #8354 (comment)) so running make style-code will happen in due time.

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Someday, it might be useful to include some designed images that combine a screenshot like this with a conceptual illustration of why these dots are special -- this could be something like an app screenshot on the left with a written function that maps a number to a color in the middle and an image of relevant tabular data on the right (just an idea).

Of course, nothing like this is required now but we might want to be on the lookout for images that end up in other parts of our docs or website and copy them back here.

Copy link
Copy Markdown
Contributor

@boundsj boundsj Mar 13, 2017

Choose a reason for hiding this comment

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

I noticed that 4 #s cause Stops to become STOPS in the generated documentation and that looks strange. Also, super nit, but we should probably format the headers consistently (add a space between the # and the first character of the text)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the generated documentation, the four #s capitalization issue combined with the formatting caused by the back ticks makes this looke like MGLINTERPOLATIONMODE with a really thin font weight.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of calling out that "exponential and interval interpolation modes" can be used on a camera function, this might be a good place to add another table that shows which functions support which modes. That is:

camera: exponential & interval
source: all of them
composite: exponential, interval, categorial (but not identity)

@jmkiley jmkiley force-pushed the jmkiley-update-guides branch from 3608a12 to 1de8180 Compare March 14, 2017 00:35
Comment thread Data-Driven Styling.md.ejs Outdated
@@ -0,0 +1 @@
wat
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Copy Markdown
Contributor Author

@jmkiley jmkiley Mar 14, 2017

Choose a reason for hiding this comment

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

oops that's from debugging! 😳

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of these variables appear to be unused. You can safely remove them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this item is complete.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It isn't necessary to specify NSColor/UIColor here, because Swift can infer the type from the type of circleColor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this entry up to keep the table alphabetized by the first column.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This table doesn't need its own subheading, but a complete sentence introducing the concept of an interpolation mode is would help to make this document flow better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Of these variables, it looks like only os is in use.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reads sort of like a release note. Let's cast this sentence in the present tense and talk about how the API includes style functions rather than introducing them.

Additionally, we have a tendency to use MGLStyleValue as an umbrella for both constants and functions. So even if we talk about the various style function subclasses, I think we should discuss the MGLStyleValue factory method that produces each kind of style function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these to-does done?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Todoooooooooo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Toooooooooodoooooooo.

@friedbunny
Copy link
Copy Markdown
Contributor

friedbunny commented Mar 17, 2017

General note: these guides would benefit greatly from linking up the method/class names to their jazzy pages.

... which is something we should look into contributing upstream to jazzy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the spaces before the colons.

screen shot 2017-03-17 at 4 59 55 pm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can also remove the initial tab indentation, as it’s indenting the entire block by one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

keys stops dictionary

Is this a dictionary of stops for multiple keys? Pluralization is a little awkward, but if the meaning is correct then it’s probably OK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the spaces before the colons and realign these lines.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Promote this to a bigger header, as in the bottom of the Runtime Styling guide.

@friedbunny
Copy link
Copy Markdown
Contributor

Are these code snippets testable? We’re dynamically inserting tested snippets into header documentation, but I’m not sure we’ve done that yet for guides.

/cc @ericrwolfe

@jmkiley jmkiley force-pushed the jmkiley-update-guides branch from 8c4a636 to e6a1dfb Compare March 17, 2017 21:23
@jmkiley
Copy link
Copy Markdown
Contributor Author

jmkiley commented Mar 17, 2017

data-driven styling reference 20170317

@ericrwolfe
Copy link
Copy Markdown
Contributor

Re: testability

I'd originally shied away from adding code snippets to the runtime styling guide out of concern for maintainability. But now that we have testable code examples we could certainly take a similar approach for the markdown guides.

We might be able to rework the original script from this PR #7337 to scan through the guides directory and replace placeholder tokens in the markdown files.

Note that the current examples script in platform/darwin/scripts/update-examples.js from #7569 is a bit more specific to work around the fact that the style layer classes are generated dynamically.

cc @1ec5

@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Mar 17, 2017

these guides would benefit greatly from linking up the method/class names to their jazzy pages.
... which is something we should look into contributing upstream to jazzy.

realm/jazzy#715

Note that the current examples script in platform/darwin/scripts/update-examples.js from #7569 is a bit more specific to work around the fact that the style layer classes are generated dynamically.

We also took that approach for the examples in the headers because jazzy documentation links to specific lines in the headers based on the source code it consumes – we didn’t want the line numbers to go out of sync because of inserted code. But that problem doesn’t affect the Markdown guides.

@jmkiley
Copy link
Copy Markdown
Contributor Author

jmkiley commented Mar 17, 2017

I opened a ticket so we can discuss how to make code snippets testable. #8461

@jmkiley jmkiley merged commit 789a49d into release-ios-v3.5.0-android-v5.0.0 Mar 17, 2017
@jmkiley jmkiley deleted the jmkiley-update-guides branch March 17, 2017 23:13
@1ec5 1ec5 removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label May 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants