Guided Tours: Add docs for our API #10007

Merged
merged 19 commits into from Jan 10, 2017

Projects

None yet

5 participants

@marekhrabe
Contributor
marekhrabe commented Dec 13, 2016 edited

Our API consists mainly of JSX components that you compose to create a tour. This PR should add explanations for their usage.

@marekhrabe marekhrabe self-assigned this Dec 13, 2016
@mcsf
Member
mcsf commented Dec 14, 2016

As discussed in Slack, +1 to splitting this off into its own docs/API.md file or similar.

Meanwhile, my other observation, once that happens, would be to separate the API description into two sections: layout components and flow-control components. Although Tour and Step are arguably very special in that they do both, I would place them under layout, like so:

  • layout: Tour, Step, ButtonRow, Link
  • flow control: Next, Quit, Continue

Oh, and I would choose a more deliberate sorting of the items, like the one I suggested above, than something alphabetical. In this particular case, I think it creates an easier-to-follow sequence, from broad to narrow (Tour → Step → Row), from simple to flexible (Next → Continue).

@mcsf

Looks good so far! Added a few comments. The mini examples are very useful, IMO

client/layout/guided-tours/docs/API.md
@@ -0,0 +1,162 @@
+# Guided Tours API
+
+Guided Tours are declared as a tree of JSX components.
@mcsf
mcsf Dec 16, 2016 Member

Since

  • JSX is just syntactic sugar;
  • I believe these are technically elements (as in: Foo is a component, <Foo /> is an element);

I think we could rephrase as

-Guided Tours are declared as a tree of JSX components.
+Guided Tours are declared in JSX as a tree of elements.
client/layout/guided-tours/docs/API.md
+### Props
+
+* `name`: (string) Unique identifier of the step.
+* `target`: (string, optional) Target which this step belongs to and will be used for positioning. This prop is value is used to look up according `[data-tip-target]` in DOM. If omitted, `<body>` will be used as target. TODO: mention querySelector hack?, is body really default target?
@mcsf
mcsf Dec 16, 2016 Member

querySelector hack

Even though it was intended as a secondary mechanism, we do use it a bit and has a few interesting strengths, like in when .some-container .some-state-like-is-valid .my-target. We should mention it.

<body> will be used

Well, in a way. positioning will grab body.getBoundingClientRect() in a few places if no target is found, but I wouldn't say that body is a "default target". Failing to provide a target will mess up positioning with most placement methods, unless they're meant for that.

@mcsf
mcsf Dec 16, 2016 Member

Oh, and the second sentence has a typo or omission somewhere — maybe s/is value/'s value/?

client/layout/guided-tours/docs/API.md
+* `name`: (string) Unique identifier of the step.
+* `target`: (string, optional) Target which this step belongs to and will be used for positioning. This prop is value is used to look up according `[data-tip-target]` in DOM. If omitted, `<body>` will be used as target. TODO: mention querySelector hack?, is body really default target?
+* `placement`: (string, optional) Placement. Possible values: 'below', 'above', 'beside', 'center', 'middle', 'right'
+* `arrow`: (string, optional) If defined, step will get arrow pointing to a direction. Available: 'top-left', 'top-center', 'top-right',
@mcsf
mcsf Dec 16, 2016 Member

Should we explain what they are? The names are kind of meaningless. Same question for placement

@mcsf
mcsf Dec 16, 2016 Member

Better yet, have a mini-gallery at the end of this paragraph with captions showing the arrows? :) Could be doable by tweaking props directly via React dev tools.

client/layout/guided-tours/docs/API.md
+* `placement`: (string, optional) Placement. Possible values: 'below', 'above', 'beside', 'center', 'middle', 'right'
+* `arrow`: (string, optional) If defined, step will get arrow pointing to a direction. Available: 'top-left', 'top-center', 'top-right',
+'right-top', 'right-middle', 'right-bottom', 'bottom-left', 'bottom-center', 'bottom-right', 'left-top', 'left-middle', 'left-bottom'
+* `style`: (object, optional) Will be used as step's inline style.
@mcsf
mcsf Dec 16, 2016 Member

I would add a word of advice, like:

  • use sparingly;
  • avoid changing the look and feel of Guided Tours;
  • useful for minor positioning adjustments, z-index, etc.
client/layout/guided-tours/docs/API.md
+
+Note that all text content needs to be wrapped in `<p>` so it gets proper styling.
+
+This is just a quick and not very useful tour. You can see more examples in [TUTORIAL.md](TUTORIAL.md) or by exploring existing tours in client/layout/guided-tours/tours.
@mcsf
mcsf Dec 16, 2016 Member

I'd make this shorter, skip the first sentence and open with For more comprehensive examples, …

client/layout/guided-tours/docs/API.md
+* `hidden`: (bool, optional) If true, this will not render anything in Step, while functionality remains.
+* `icon`: (string, optional) Name of Gridicon to show in custom message.
+* `when`: (function, optional) Redux selector. Once it evaluates to true, tour will advance to `step`.
+
@mcsf
mcsf Dec 16, 2016 Member

Missing prop is children. It's good to make it clear that it is possible to use children, but also (importantly) that it is not needed, as we have good defaults — with and without icon

client/layout/guided-tours/docs/API.md
+- text
+- text + props.icon
+- custom content
+
@mcsf
mcsf Dec 16, 2016 Member

With no introduction, I find this enumeration confusing

client/layout/guided-tours/docs/API.md
+// continue when user clicks DOM element with html attribute `data-tip-target="my-sites"`
+<Continue step="next-step" click target="my-sites" />
+// continue when Redux selector evaluates to true (in this case after user opens preview)
+<Continue step="next-step" when={ isPreviewShowing } />
@mcsf
mcsf Dec 16, 2016 Member

Not sure what the place to say this is, but authors should be aware that when will only work if something triggers a re-render of the step and that the proper way to set this up is to add the relevant action types to actionLog's whitelist.

client/layout/guided-tours/docs/API.md
+// with default label
+<Next step="next-step" />
+// or with a custom one
+<Next step="next-step">Custom Label</Next>`
@mcsf
mcsf Dec 16, 2016 Member

Add translate( )

@mcsf
mcsf Dec 16, 2016 Member

Same for Quit

client/layout/guided-tours/docs/API.md
+
+## All-in example
+
+Before we go into full details, let's look at this example. It includes most possible things you can use.
@lsinger
lsinger Dec 23, 2016 Contributor

Make sure to say somewhere it's an example for a Step, and what a Step is. I first thought OK, I'll now get to see an example of a full tour.

client/layout/guided-tours/docs/API.md
+
+## Tour
+
+Tour is a React component that declares the top-level of a tour. It defines conditions for starting a tour and contains `<Step>` elements as children.
@lsinger
lsinger Dec 23, 2016 Contributor

Tour doesn't appear in the example above. Again, somewhat confusing.

client/layout/guided-tours/docs/API.md
+
+## Step
+
+Step is a React component that defines a single Step of a tour. It is represented as dark box on top of Calypso UI. Step can be positioned in many ways in relation to any DOM node in Calypso that is marked with `[data-tip-target]` attribute.
@lsinger
lsinger Dec 23, 2016 Contributor

target also accepts a CSS selector, we'll then use the first match.

@lsinger
lsinger Dec 23, 2016 Contributor

ah, you say it below. maybe remove implementation details here and just say "DOM node in Calypso".

@marekhrabe
marekhrabe Dec 23, 2016 Contributor

It's mentioned in the prop list just few lines below. Should it also be here?

@lsinger
lsinger Dec 23, 2016 Contributor

see my second comment. :)

@lsinger
lsinger Dec 23, 2016 Contributor

I'm still going through it, you know. :)

client/layout/guided-tours/docs/API.md
+
+### Props
+
+* `name`: (string) Unique identifier of the step.
@lsinger
lsinger Dec 23, 2016 Contributor

, used for ... (next)

@lsinger
lsinger Dec 23, 2016 Contributor

also, init and finish are magic step names I think.

@marekhrabe
marekhrabe Dec 23, 2016 Contributor

had no idea, thanks! btw: what if the tour has only one step? is it init?

@marekhrabe
marekhrabe Dec 23, 2016 Contributor

it seems like the only magical name is init, isn't it? finish is not detected anywhere as far as I'm looking right

@lsinger
lsinger Jan 4, 2017 Contributor

didn't I reply to this before? weird. in any case, you're right. finish was used previously for styling, but isn't used anymore.

client/layout/guided-tours/docs/API.md
+### Props
+
+* `name`: (string) Unique identifier of the step.
+* `target`: (string, optional) Target which this step belongs to and will be used for positioning. Value of this prop is used to look up according `[data-tip-target]` in DOM. If you start this value with `.` (dot), it will be evaluated as a query selector, so you can select elements that have no `[data-tip-target]` defined.
@lsinger
lsinger Dec 23, 2016 Contributor

also works as a selector if a space is in there. maybe add # to the check. the . is just an indicator for "hey, classname selector".

@lsinger
lsinger Dec 23, 2016 Contributor

i.e. it's passed along as is. the . isn't removed.

client/layout/guided-tours/docs/API.md
+### Props
+
+* `name`: (string) Unique identifier of the step, used for addressing a step from `Next` or `Continue`.
+* `target`: (string, optional) Target which this step belongs to and will be used for positioning. Value of this prop is used to look up according `[data-tip-target]` in DOM. If you start this value with `.` (dot), it will be evaluated as a query selector, so you can select elements that have no `[data-tip-target]` defined.
@lsinger
lsinger Dec 23, 2016 Contributor

if you start with . or if it contains a space, those we use as indicators to figure out whether it's maybe a selector. we might also want to add # to that.

client/layout/guided-tours/docs/API.md
+
+Guided Tours are declared in JSX as a tree of elements. All of them are available as named exports from `client/layout/guided-tours/config-elements`.
+
+## All-in example
@lsinger
lsinger Dec 23, 2016 Contributor

instead of having the Step here, move it to ## Step below.

here you might want to add some more general advice, like, a Tour is a series of Step elements. the tour gets triggered either if selectors match or when it's forced to start through a query argument. (document the query arg somewhere, maybe where you document the tour name prop) the user can then step through the tour and get Calypso aspects explained. can be interactive in that Step element can wait for a user to do certain actions such as clicking a menu item. we can also skip tour steps depending on selectors, e.g. skip a single Step if that one doesn't work well on mobile.

client/layout/guided-tours/docs/API.md
+* `placement`: (string, optional) Placement. Possible values: 'below', 'above', 'beside', 'center', 'middle', 'right'
+* `arrow`: (string, optional) If defined, step will get arrow pointing to a direction. Available: 'top-left', 'top-center', 'top-right',
+'right-top', 'right-middle', 'right-bottom', 'bottom-left', 'bottom-center', 'bottom-right', 'left-top', 'left-middle', 'left-bottom'
+* `style`: (object, optional) Will be used as step's inline style. Use sparingly and with caution for minor tweaks in positioning or z-indexes and avoid changing the look and feel of Guided Tours. If you use this is a way that could benefit all Guided Tours globally, please consider creating an issue.
@lsinger
lsinger Dec 23, 2016 Contributor

give example so format becomes obvious

+### Props
+
+* `step`: (string) Name of the step the tour will advance to.
+* `target`: (string, optional) Name of `[data-tip-target]` that would be watched.
@lsinger
lsinger Dec 23, 2016 Contributor

or CSS selector

@lsinger
lsinger Dec 23, 2016 Contributor

right?

@lsinger
lsinger Dec 23, 2016 Contributor

not sure actually.

@mcsf
mcsf Jan 2, 2017 Member

I expect so, as it's calling targetForSlug internally.

@marekhrabe
marekhrabe Jan 4, 2017 Contributor

I think we might want to extract docs for targetForSlug somehow and link to it from several places, where we explain target

@lsinger
lsinger Jan 4, 2017 Contributor

Yeah, maybe just introduce something like a pseudo-datatype "Target" that we only use in the docs.

client/layout/guided-tours/docs/API.md
+There are currently two ways to declare the condition to continue the tour.
+
+- Binding an `onClick` listener to a DOM node marked as `[data-tip-target]`
+- Redux selector function that evaluates to true in order to advance the tour
@lsinger
lsinger Dec 23, 2016 Contributor

well, or just add a Next button

@marekhrabe
marekhrabe Jan 4, 2017 Contributor

but is this really the right place to do that? this section explains how to use Continue in different ways and Next is not one of them

@lsinger
lsinger Jan 4, 2017 Contributor

"There are currently two ways to declare the condition to continue the tour" is a more general statement than "here's how you use Continue", which is why I'd add the Next button (with mentioning the caveat that it's not for use with Continue, but an alternative) — or reword the "two ways" sentence.

@mcsf
mcsf approved these changes Jan 9, 2017 View changes

Looks great! Highlighted a couple of things to fix.

+
+### Props
+
+* `primary` (bool, optional) If true, button will be rendered as primary. Use only if Quit is the only available action in that step.
@mcsf
mcsf Jan 9, 2017 Member

While we're here, let's add Leif's subtle prop from #10035.

client/layout/guided-tours/docs/API.md
+<Quit />
+
+// with a custom label
+<Quit>{ translate( 'Custom Label' ) }</Quit>`
@mcsf
mcsf Jan 9, 2017 Member

There's a stray backtick at the end of the line

client/layout/guided-tours/docs/API.md
+
+### Example
+
+```
@mcsf
mcsf Jan 9, 2017 Member

Minor: make that a ```js

@lsinger lsinger merged commit be177de into master Jan 10, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/i18n 0 new strings. Translations: nan% translated.
@lsinger lsinger deleted the add/gt-component-docs branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment