NUX: Make tips appear inline by noisysocks · Pull Request #16582 · WordPress/gutenberg

noisysocks · 2019-07-15T04:01:40Z

Implements part of #16315.

Improve the usability of NUX tips by making them appear as inline notices (via a new InlineTip component) rather than floating popovers.

To test:

Create a new post
Click More tools & options
Select Options
Check Enable Tips

talldan

Did a review 👍

talldan · 2019-07-15T07:21:25Z

-	// Options modal, which looks totally weird.
-	DeferredOption
-);
+)( BaseOption );


kjellr · 2019-07-15T12:21:25Z

This is looking great so far!

I hadn't realized that Notices have such super-tall top and bottom margins. 😄 I separately opened #16589 to clean that up.

noisysocks · 2019-07-19T05:31:08Z

 codeEditingEnabled                     string        Whether or not the user can switch to the code editor
- \_\_experimentalCanUserUseUnfilteredHTML string        Whether the user should be able to use unfiltered HTML or the HTML should be filtered e.g., to remove elements considered insecure like iframes.
+ **experimentalCanUserUseUnfilteredHTML string        Whether the user should be able to use unfiltered HTML or the HTML should be filtered e.g., to remove elements considered insecure like iframes.
+ **experimentalEnableTips               boolean       Whether or not tips aimed at new users should appear in the UI.


I don't know why npm run docs:build has replaced the __ with ** 😕

mapk · 2019-07-31T15:02:40Z

This is looking really good. I just tested this today and noticed a few things.

When closing a tip, I expected the message to appear inline like this: Revise the approach to NUX tips #16315 (comment) But maybe we're not going this route? Or not for version 1?
The appearance of a complete alert message overlay begins to get obstructive when I close two tips one after the other. This may be an edge case though?

The language on the button didn't jive with my expectations. Right now there's a "Cancel" button. By clicking Cancel, I expected the tip to remain on screen and cancel my action of trying to close it. Maybe it should say, "No" and "Yes" ? I'm not sure here. Maybe "Close" and "Disable Tips"?

noisysocks · 2019-08-01T01:17:23Z

@mapk: I'm working off this comp which has the confirmation as a modal. Currently using an alert() because of some technical difficulties that I'm working through. Stay tuned! 🙂

Improve the usability of NUX tips by making them appear as inline notices rather than floating popovers. Adds a new InlineTip component to accomplish this.

noisysocks · 2019-08-02T05:13:12Z

This should be ready for a final review! 🙏

talldan

I was really close to approving, but then I spotted some issues in testing, probably all related. 😭

When opening the confirmation modal from the insertion menu, focus isn't moved to the modal, you can see it's still in the inserter.
The modal can be closed by clicking outside. I feel like this is too easy to do given how small the modal is. It's also unusual for these kind of confirmation modals to be closable in this kind of way. One other thing is that the inserter menu closes as well when this happens:
That leads onto the next issue, which is that focus isn't transferred back to the right place when the modal is closed using the escape key (I imagine because it's not there any more):

I wonder if it's because the insertion menu has some logic for trapping keyboard input (I might've written it 😬): https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/inserter/menu.js#L242-L247

Might also be a good use case for IgnoreNestedEvents, not sure as I haven't used that component much.

talldan · 2019-08-02T06:46:52Z

+	const classes = classnames(
+		className,
+		'components-notice',
+		status ? 'is-' + status : undefined,


It's really minor, and I don't care so much if it gets merged without it, but I notice this has changed and I think it could go in the object below.

Also wondering if status should be kebabbed for ultra safety:

[ `is-${ kebabCase( status ) }` ]: status,

talldan · 2019-08-02T07:29:28Z

->
-  <div
-    className="components-notice__content"
+<Fragment>


This snapshot now seems a bit pointless!

talldan · 2019-08-02T07:31:48Z

- * WordPress dependencies
- */
-import { Notice } from '@wordpress/components';
+import { shallow, mount } from 'enzyme';


I'm wondering if it is actually worth nuking these tests. E2E tests seem to cover the same ground.

talldan · 2019-08-02T07:37:38Z

 	return (
 		<>
+			{ enableTips && (
+				<InlineTip tipId="core/editor.blockInspector" className="block-editor-block-inspector__tip">


Should it be "core/block-editor.blockInspector" now? Would there be issues with localstorage keys if that changed?

talldan · 2019-08-02T07:45:32Z

+		const { disableTips, dismissTip } = dispatch( 'core/nux' );
+		return {
+			onDismissTip: () => dismissTip( tipId ),
+			onDisableTips: disableTips,


Nitpick - The prefix 'on' seems like it could be changed. Usually I'd expect something prefixed with 'on' to be an event handler prop on a component, but this is an action dispatch and most of those just start with a verb. I found the usage in some of the functions above looked a bit unusual.

mapk · 2019-08-06T01:17:21Z

One more little tidbit to note... I just noticed the hover color for the close X icon is red which feels unnecessary. Is this a pattern being pulled from somewhere? If not, let's just make the icon darker on hover. The red feels out of place here.

kjellr · 2019-08-06T14:56:25Z

One more little tidbit to note... I just noticed the hover color for the close X icon is red which feels unnecessary. Is this a pattern being pulled from somewhere? If not, let's just make the icon darker on hover. The red feels out of place here.

Yeah, that's part of the Notice component. I've started #16926 to fix that.

noisysocks · 2019-08-06T23:03:56Z

Hitting the ⏸ pause button on this until #16592 is resolved.

kjellr · 2019-09-04T13:18:57Z

Quick note on this one: we'll keep this PR closed, and will pick up the new changes proposed in #16315 (comment):

Remove the current NUX tips.
Implement the new welcome modal described in that comment.

noisysocks added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Feature] NUX Anything that impacts the new user experience [Package] NUX labels Jul 15, 2019

noisysocks requested review from aduth, ajitbohra, ellatrix, gziolo, jorgefilipecosta, nerrad, ntwb, talldan and youknowriad as code owners July 15, 2019 04:01

noisysocks commented Jul 15, 2019

View reviewed changes

Comment thread packages/block-editor/package.json Outdated

noisysocks force-pushed the add/inline-tips branch from 076a1e1 to 2011a87 Compare July 15, 2019 04:28

noisysocks commented Jul 15, 2019

View reviewed changes

Comment thread packages/nux/src/components/inline-tip/index.js Outdated

talldan reviewed Jul 15, 2019

View reviewed changes

noisysocks commented Jul 19, 2019

View reviewed changes

kjellr mentioned this pull request Jul 31, 2019

Add a help panel to the inserter available in all blocks #16813

Merged

noisysocks added the [Status] In Progress Tracking issues with work in progress label Aug 1, 2019

noisysocks added 6 commits August 2, 2019 12:18

NUX: Make tips appear inline

18ab2e9

Improve the usability of NUX tips by making them appear as inline notices rather than floating popovers. Adds a new InlineTip component to accomplish this.

InlineTip: Link to the Notice component's documentation

f45b046

InlineTip: Mark as experimental

51baead

InlineTip: Use more accurate test description

bdd6473

BlockEditor: Do not show InlineTips by default

609a09c

InlineTip: Use Modal instead of window.confirm()

b1270ca

noisysocks force-pushed the add/inline-tips branch from 48652f9 to 74e3205 Compare August 2, 2019 04:30

noisysocks requested a review from chrisvanpatten as a code owner August 2, 2019 04:30

noisysocks removed the [Status] In Progress Tracking issues with work in progress label Aug 2, 2019

InlineTip: Don't show confirmation if a tip has already been dismissed

7333838

noisysocks force-pushed the add/inline-tips branch from 74e3205 to 7333838 Compare August 2, 2019 04:37

noisysocks requested a review from mkaz as a code owner August 2, 2019 04:37

talldan requested changes Aug 2, 2019

View reviewed changes

noisysocks added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Aug 6, 2019

noisysocks closed this Aug 23, 2019

noisysocks mentioned this pull request Aug 23, 2019

Revise the approach to NUX tips #16315

Closed

noisysocks deleted the add/inline-tips branch August 23, 2019 05:15

Uh oh!

Conversation

noisysocks commented Jul 15, 2019

Uh oh!

Uh oh!

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

talldan Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kjellr commented Jul 15, 2019

Uh oh!

noisysocks Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

mapk commented Jul 31, 2019

Uh oh!

noisysocks commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noisysocks commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talldan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

talldan Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

talldan Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

talldan Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

mapk commented Aug 6, 2019

Uh oh!

kjellr commented Aug 6, 2019

Uh oh!

noisysocks commented Aug 6, 2019

Uh oh!

kjellr commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noisysocks commented Aug 1, 2019 •

edited

Loading

noisysocks commented Aug 2, 2019 •

edited

Loading

talldan left a comment •

edited

Loading

talldan Aug 2, 2019 •

edited

Loading