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鈥檒l 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

Conversation

Projects
None yet
@noisysocks
Member

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鈥攖he current step should be maintained
  4. Dismiss the tips
  5. Refresh the page鈥攖he 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

This comment has been minimized.

Member

noisysocks commented May 8, 2018

},
preview: {
step: 3,
text: __( 'Click 鈥楶review鈥 to load a preview of this page, so you can make sure you鈥檙e happy with your blocks. ' ),

This comment has been minimized.

@Soean

Soean May 8, 2018

Member

Whitespace at the end of the sentence.

This comment has been minimized.

@noisysocks

noisysocks May 9, 2018

Member

Thanks, removed in 4b9ff4d.

@youknowriad

This comment has been minimized.

Contributor

youknowriad 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 :)

@noisysocks

This comment has been minimized.

Member

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鈥擨'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 changed the title from [WIP] New User Guide to New User Experience (NUX) May 22, 2018

@noisysocks noisysocks requested a review from karmatosed May 22, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented May 22, 2018

This is in a reviewable state now鈥擨've updated the PR description 馃惗馃寛

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 22, 2018

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

This comment has been minimized.

michelleweber commented May 22, 2018

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 }>

This comment has been minimized.

@youknowriad

youknowriad May 22, 2018

Contributor

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.

This comment has been minimized.

@noisysocks

noisysocks May 23, 2018

Member

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

@noisysocks

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

youknowriad commented May 23, 2018

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 />

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

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

This comment has been minimized.

@noisysocks

noisysocks May 23, 2018

Member

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

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

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.

This comment has been minimized.

@noisysocks

noisysocks May 23, 2018

Member

I鈥檒l 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鈥檛 dismiss
  • Must appear above other popovers

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

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鈥檛 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

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

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

@karmatosed

This comment has been minimized.

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

This comment has been minimized.

Member

noisysocks commented May 25, 2018

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.

@@ -213,6 +213,11 @@ class Popover extends Component {
isMobile,
} = this.state;
let { noArrow = false } = this.props;
if ( xAxis === 'center' && yAxis === 'middle' ) {

This comment has been minimized.

@youknowriad

youknowriad May 25, 2018

Contributor

Not certain about this? in which case will have both centered?

This comment has been minimized.

@noisysocks

noisysocks May 28, 2018

Member

There's no case in Gutenberg but a third party could use the component this way.

Perhaps a console.warn would be better? I just want to explicitly declare somewhere that using Popover with position="middle center" noArrow={ false } is likely to look weird.

popoverTop = topAlignment.popoverTop;
} else {
popoverTop = bottomAlignment.popoverTop;
}

This comment has been minimized.

@youknowriad

youknowriad May 25, 2018

Contributor

Thanks for the updates 馃憤 Can we add some unit tests for the "middle" alignment?

This comment has been minimized.

@noisysocks
@noisysocks

This comment has been minimized.

Member

noisysocks commented May 29, 2018

OK, I think that this is in a good place now. I've updated the PR description. It's on the larger side (977 lines) but a lot of that is documentation. Let me know if you'd prefer for me to split it up.


@karmatosed: I struggled a little with getting the dots to appear exactly how you want. To keep the code simple, we should try to position the dot in the same way for every control. I think it's best that we pick a single number of pixels between the centre of the dot and the left/right edge of the button. I went with 4px which, since the dot is 8px in diameter, means that the dot rests flush against the edge of the button.

To illustrate what I mean:

margin 1

margin 2

This is how it looks:

dot-inserter

dot-settings

dot-preview

Let me know if this is OK or if you'd like to change the number of pixels or the approach I'm taking.

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 29, 2018

The only one I caution on for position is:

2018-05-29 at 16 45

I think for now @noisysocks that positioning works at least to get this into 3.0. I do think we need to explore a variation on close to not lead to a next tip that doesn't exist. Can we change the last link to be something like 'Let's get started writing'?

My only other thought is whether we could increase the text size of the Tip, we can maybe review this though.

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 29, 2018

Here is a video for those who want to follow along and review:

tips

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 29, 2018

Here is what it looks like on non-desktop screens:

tipsmobile

noisysocks added some commits May 31, 2018

Remove aria-modal property from DotTip
Safari 11 has a weird bug when this property is used.
Implement New User Guide
Adds a series of floating modal 'tips' which introduces a new user to
the editor. These tips can be advanced one by one or dismissed
alltogether. If dismissed, they will never show again.
Fix DotTip focus issue
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.
NUX: Only show inserter tip in DefaultBlockAppender
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 noisysocks merged commit e809505 into master Jun 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 7, 2018

Thanks all! 鉂わ笍

@noisysocks noisysocks deleted the try/nux-tips branch Jun 7, 2018

@afercia

This comment has been minimized.

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

This comment has been minimized.

Member

noisysocks commented Jun 7, 2018

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

This comment has been minimized.

Member

noisysocks commented Jun 7, 2018

@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

This comment has been minimized.

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 }>

This comment has been minimized.

@aduth

aduth Jun 19, 2018

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

This comment has been minimized.

@noisysocks

noisysocks Jun 20, 2018

Member

When do you see that warning, @aduth?

This comment has been minimized.

@aduth

aduth Jun 20, 2018

Member

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 鈥楢dd block鈥 to insert different kinds of content鈥攖ext, 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() }

This comment has been minimized.

@aduth

aduth Jul 30, 2018

Member

Why do we stop propagation.

This comment has been minimized.

@noisysocks

noisysocks Jul 30, 2018

Member

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

This comment has been minimized.

@aduth

aduth Jul 31, 2018

Member

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)

This comment has been minimized.

@noisysocks

noisysocks Jul 31, 2018

Member

馃榾 yep, you're right: #8339

@afercia

This comment has been minimized.

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

This comment has been minimized.

Member

noisysocks commented Jul 31, 2018

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

This comment has been minimized.

Contributor

afercia commented Jul 31, 2018

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

@aduth

This comment has been minimized.

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