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

New User Experience (NUX) #6631

Merged
merged 21 commits into from
Jun 7, 2018
Merged

New User Experience (NUX) #6631

merged 21 commits into from
Jun 7, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented May 8, 2018

🕺 What this is

Closes #3670. Blocked by #7038.

This implements the NUX flow outlined by @karmatosed in #3670 (comment):

When a new user opens Gutenberg, they're presented with a guide that points out some useful page elements:

nux

If they dismiss the guide, it won't ever show again:

nux-dismiss

🤔 How it works

  • A DotTip component renders a tip bubble independently of anything else
    • This uses Popover, which now supports having its yAxis set to middle
  • The triggerGuide() action lets you associate a series of tips into a single guide
  • The isTipVisible() and getAssociatedGuide() selectors return information about which step in a guide the user is in
  • The dismissTip() and disableTips() actions control dismissing a single tip (and stepping through the guide) and disabling tips entirely
  • All of this functionality is contained within a new nux module with the intention that these components should be made available in other areas of the WP Admin

📋 Testing

There's unit tests included. Manual testing can be done by:

  1. Create a new post
  2. View and step through the tips
  3. Refresh the page—the current step should be maintained
  4. Dismiss the tips
  5. Refresh the page—the tips should no longer appear

✍️ Copy

I'm still waiting on the final copy for each tip. There was some discussion about this in #3670.

@noisysocks noisysocks added [Status] In Progress Tracking issues with work in progress [Feature] NUX Anything that impacts the new user experience labels May 8, 2018
@noisysocks
Copy link
Member Author

cc. @karmatosed

},
preview: {
step: 3,
text: __( 'Click ‘Preview’ to load a preview of this page, so you can make sure you’re happy with your blocks. ' ),
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace at the end of the sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, removed in 4b9ff4d.

@youknowriad
Copy link
Contributor

Just a quick comment :) I'd love if the nux is built as a separate module to be reusable in all WP and not only the editor context. See #4173 for prior art. I started this module and put it on hold because of priorities :)

@noisysocks
Copy link
Member Author

noisysocks commented May 14, 2018

Just a quick comment :) I'd love if the nux is built as a separate module to be reusable in all WP and not only the editor context. See #4173 for prior art. I started this module and put it on hold because of priorities :)

Oh, nice. I didn't see your PR! It's reassuring that we both arrived at some of the same decisions e.g. rendering tips at the point where they're shown and giving each one a unique ID.

A seperate module is a good idea—I'll move NewUserTip into a new nux module.

Unsure how flexible we want to make this module right now, though. My priority is to get something minimal merged ASAP.

@noisysocks noisysocks force-pushed the try/nux-tips branch 6 times, most recently from 9897c9c to ef9e3b7 Compare May 21, 2018 22:38
@noisysocks noisysocks changed the title [WIP] New User Guide New User Experience (NUX) May 22, 2018
@noisysocks noisysocks added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. and removed [Status] In Progress Tracking issues with work in progress labels May 22, 2018
@noisysocks
Copy link
Member Author

This is in a reviewable state now—I've updated the PR description 🐶🌈✨

@karmatosed
Copy link
Member

This is coming on well. I have some initial bits of feedback and then I think we can really move into polish.

  • I feel the width could be increased on the tips we need to of course adapt to mobile, but the small size is making an issue with visuals, this isn't a tooltip and shouldn't have those size restrictions.

2018-05-22 at 17 58

  • Can we point out the '+' in the post content over being all at the top bar like the mocks?

2018-05-22 at 18 01

  • Can we not have it ever go over the toolbar?

2018-05-22 at 17 59

A couple of things, I would like it to be a link not a button, the button to move to next tip. That was a link on purpose to make it more conversational than a required UI. I also would say let's iterate on what it says 'got it' feels a little too casual. @michelleweber what would you suggest for the words to move to next tip?

@michelleweber
Copy link

To be super-clear, I'd probably go with "Next" or "What's next?" on a button. If you want it to be a text link instead, either "What's next?" or "Great! What's next?" or something like "Tell me more!"

@@ -30,7 +31,11 @@ function HeaderToolbar( { hasFixedToolbar, isLargeViewport } ) {
className="edit-post-header-toolbar"
aria-label={ __( 'Editor Toolbar' ) }
>
<Inserter position="bottom right" />
<Inserter position="bottom right">
<GuideTip guideID="core/editor" step={ 1 }>
Copy link
Contributor

@youknowriad youknowriad May 22, 2018

Choose a reason for hiding this comment

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

Do we really want to step prop? I'm thinking these tips should be autonomous because I can see that some may show up where others not depending on different criterias:

  • The blocks count
  • Whether it's the first time we're using gutenberg
  • Whether it's the "n" time we use gutenberg (more experienced tips)
  • Whether we open a classic block's post (tip to indicate that it's a classic block that can be converted)
  • ...

My idea is that tips have only ids, I can even see several tips showing at the same time some time.
I think we probably need guides too but I wonder if it's better to use props for this or another API like:

const guide = [ tipId1, tipId2];
wp.dispatch( 'core/nux' ).triggerGuide( guide ):

I'm just thinking out loud here, nothing very well thought.

Copy link
Member Author

@noisysocks noisysocks May 23, 2018

Choose a reason for hiding this comment

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

Agree that autonomous tips would be more future-proof—I'll have a play with something like triggerGuide().

@noisysocks
Copy link
Member Author

noisysocks commented May 23, 2018

Thanks @karmatosed! In 66ba2ac I've:

  • Increased the width of tips by 100px
  • Made tips not appear over the toolbar
  • Styled the next button as a link

Let me know what you think.

Can we point out the '+' in the post content over being all at the top bar like the mocks?

It's difficult because that button doesn't appear until you hover your mouse over the block placeholder. I'll see what I can do, though.

Done! 👌

To be super-clear, I'd probably go with "Next" or "What's next?" on a button. If you want it to be a text link instead, either "What's next?" or "Great! What's next?" or something like "Tell me more!"

My issue with copy that contains the word 'next' is that, when you're on the last tip, there isn't a 'next tip' to go to. I've changed the copy to 'Tell me more!' for now but will work on making it so that we can have different copy for the button when it's the last tip.

@youknowriad
Copy link
Contributor

Trying this. I really like the design of the tips 👏

I was wondering what should happen when I dismiss the original "welcome tip". I personally expect the other tips to still show up (closed by default) when necessary while it seems that currently all the "guide" is dismissed. Going this road means we also need a "Dismiss all tips" link or preference somewhere.

@@ -94,6 +95,7 @@ function Layout( {
}
<Popover.Slot />
<PluginArea />
<GuideTip.Slot />
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for requiring another Slot, why it's not using the Popover component instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved away from Popover as there’s some differences in appearance and behaviour. We probably will want to extract common functionality into a shared component e.g. PopoverBase. I’m wary of introducing this abstraction while we’re experimenting with the NUX UI, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just made some adjustments to the Popover component and I think it's great in resolving all the positioning/size issues. I'd love to know what are those difference to see if we can bake them into the Popover component. From the screenshots it looks very similar to the popover component. I believe what's missing is the support of the "center" position in the yAxis. We only support it in the xAxis.

Copy link
Member Author

@noisysocks noisysocks May 23, 2018

Choose a reason for hiding this comment

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

I’ll check out your adjustments. When I last looked, the differences were:

  • Blue dot instead of an arrow
  • Should appear to the left or right of the anchor point instead of above or below
  • Clicking outside shouldn’t dismiss
  • Must appear above other popovers

Copy link
Contributor

Choose a reason for hiding this comment

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

Blue dot instead of an arrow

The blue dot should be the anchor (outside the popover)

Should appear to the left or right of the anchor point instead of above or below

Yes, that's what's missing the "center" support for the yAxis. I believe we can add it.

Clicking outside shouldn’t dismiss

This should work, maybe a bug somewhere

Must appear above other popovers

Popovers take classNames, I believe we can adjust the z-index for nux popovers

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm adding a noArrow prop to this one that could also help #6911 (comment)

@karmatosed
Copy link
Member

karmatosed commented May 24, 2018

@noisysocks thanks for the updates, it's getting really solid - we're so close! I do have one adjustment that will effect all I see, can we have more space between the pulsing dot and area it's indicating? Here are some adjusted mocks - the last one in each group is the one I'd like to change to.

artboard1

artboard 2

artboard 3

@noisysocks
Copy link
Member Author

Thanks for the useful feedback @youknowriad! I've changed this to use Popover and a triggerGuide API that keeps the Tip component relatively isolated. Let me know what you think.

Re-adding the In Progress label because I need to fix some mobile issues that I introduced when moving over to using Popover.

@noisysocks noisysocks added the [Status] In Progress Tracking issues with work in progress label May 25, 2018
Safari 11 has a weird bug when this property is used.
The first DotTip should receive focus when the page loads, and focus
should not be on the X button.

Also improve the note explaining our temporary position workaround.
Only display the first DotTip in the new user guide in the
DefaultBlockAppender that appears in a new post. This prevents the guide
from appearing in an existing post that contains empty blocks.
@noisysocks
Copy link
Member Author

Thanks all! ❤️

@noisysocks noisysocks deleted the try/nux-tips branch June 7, 2018 03:29
@afercia
Copy link
Contributor

afercia commented Jun 7, 2018

Sorry for late reply:

What should be focused, @afercia? Just the modal itself? Right now, Popover focuses the first focusable element in the container element which is the button.

Considering there's also the order issue:

Close button position: visual order should always match DOM order; currently, this button is at the end of the tip content but then positioned with CSS at the top

moving the "X" button at the beginning of the DOM would solve it, as it would be the first focusable element, followed by the content. Should I open a separate issue? @noisysocks

@noisysocks
Copy link
Member Author

moving the "X" button at the beginning of the DOM would solve it, as it would be the first focusable element, followed by the content. Should I open a separate issue? @noisysocks

No need for an issue, I can do this in a follow-up PR 🙂

@noisysocks
Copy link
Member Author

@afercia: Hm, the issue with moving the X button to the beginning of the DOM is that the tooltip is then shown when a new user opens Gutenberg for the first time:

nux-focus

That's a little jarring. Also, I think that we want the 'See next tip' button to be initially focused as it is the primary action that we want to encourage. That is, we want to encourage folks to advance through the tutorial and not dismiss the guide entirely.

Does that sound right to you? Do you have any alternative suggestions?

@afercia
Copy link
Contributor

afercia commented Jun 7, 2018

Hm @noisysocks well "visual order should match DOM order" it's part of a specific WCAG requirement "when navigation sequences affect meaning or operation". On the other hand, there may be different orders that reflect logical relationships in the content so I guess there may be different opinions.

Reference:
Success Criterion 2.4.3 Focus Order
https://www.w3.org/TR/WCAG21/#focus-order

Understanding Success Criterion 2.4.3: Focus Order
https://www.w3.org/WAI/WCAG21/Understanding/focus-order.html

Making the DOM order match the visual order
https://www.w3.org/WAI/WCAG21/Techniques/css/C27

The important bit is that focus order should not appear to "jump around" randomly. That said, there's a technique for modals that can be used to make screen readers announce the content automatically using aria-describedby:

<Popover
	ref={ this.popoverRef }
	className="nux-dot-tip"
	position="middle right"
	noArrow
	focusOnMount
	role="dialog"
	aria-label={ __( 'Gutenberg tips' ) }
	onClose={ onDismiss }
	onClick={ ( event ) => event.stopPropagation() }
	aria-describedby="tip-content-id-here"
>
	<p id="tip-content-id-here">{ children }</p>

Of course the ID should be unique. Multiple elements can be referenced too: aria-describedby="first-element-id second-element-id"

Aside: I've just noticed that pressing Escape navigates to the next tip, not sure why. The expected behavior is that it should close the tip.

>
<p>{ children }</p>
<p>
<Button isLink onClick={ onDismiss }>
Copy link
Member

Choose a reason for hiding this comment

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

Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>.
    in button
    in p (created by DotTip)

Copy link
Member Author

Choose a reason for hiding this comment

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

When do you see that warning, @aduth?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's with the default block appender Inserter tip, whose children are rendered within an IconButton:

<Inserter position="top right">
<DotTip id="core/editor.inserter">
{ __( 'Welcome to the wonderful world of blocks! Click ‘Add block’ to insert different kinds of content—text, images, quotes, video, lists, and much more.' ) }
</DotTip>
</Inserter>

<IconButton
icon="insert"
label={ __( 'Add block' ) }
onClick={ onToggle }
className="editor-inserter__toggle"
aria-haspopup="true"
aria-expanded={ isOpen }
>
{ children }
</IconButton>
) }

role="dialog"
aria-label={ __( 'Gutenberg tips' ) }
onClose={ onDismiss }
onClick={ ( event ) => event.stopPropagation() }
Copy link
Member

Choose a reason for hiding this comment

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

Why do we stop propagation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tips are often nested within buttons. We stop propagation so that clicking on a tip doesn't result in the button being clicked.

Copy link
Member

@aduth aduth Jul 31, 2018

Choose a reason for hiding this comment

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

Tips are often nested within buttons. We stop propagation so that clicking on a tip doesn't result in the button being clicked.

This reads perfectly as an inline code comment 😄 (We should be cautious about our use of stopPropagation. This is a reasonable one, though not clear to the future maintainer why it exists)

Copy link
Member Author

Choose a reason for hiding this comment

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

😀 yep, you're right: #8339

@afercia
Copy link
Contributor

afercia commented Jul 31, 2018

Tips are often nested within buttons.

Yes and since the tips contain buttons, and they're initially within buttons, as noted in #7510 this produces a Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button> (at least for me)

@noisysocks
Copy link
Member Author

Very weird that that warning happens. The React component is a descendent a React <button>, element, sure, but the actual rendered DOM nodes are never within any <button> DOM nodes since we are using portals. I wonder if there's a React bug causing the warning.

@afercia
Copy link
Contributor

afercia commented Jul 31, 2018

Not sure, maybe validateDOMNesting does a static analysis before the portal kicks in? Just a guess

@aduth
Copy link
Member

aduth commented Jul 31, 2018

sure, but the actual rendered DOM nodes are never within any <button> DOM nodes since we are using portals.

Important to note that Popover doesn't force the use of portals. It also allows an inline rendering mode when the context is not provided (see render).

It's possible this could be related to findings described at #6799, where there's an initial render of Popover without the slot portaling.

Or, as @afercia mentioned, it may well be that React performs its analysis on the virtual shape, not necessarily what's rendered to the DOM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] NUX Anything that impacts the new user experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet