NUX: Make tips appear inline #16582

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

packages/block-editor/package.json

packages/nux/src/components/inline-tip/index.js

talldan

Did a review 👍

packages/block-editor/package.json

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

packages/edit-post/src/components/options-modal/options/enable-tips.js

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


packages/nux/src/components/inline-tip/test/index.js

packages/nux/src/components/inline-tip/index.js

packages/nux/src/components/inline-tip/README.md

packages/nux/src/components/inline-tip/index.js

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

packages/block-editor/README.md

@@ -370,7 +370,8 @@ The default editor settings
 bodyPlaceholder                        string        Empty post placeholder
 titlePlaceholder                       string        Empty title placeholder
 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

packages/components/src/notice/index.js

+	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

packages/nux/src/components/inline-tip/test/__snapshots__/index.js.snap

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


This snapshot now seems a bit pointless!

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

packages/nux/src/components/inline-tip/test/index.js

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

packages/block-editor/src/components/block-inspector/index.js

@@ -51,6 +53,11 @@ const BlockInspector = ( {

 	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

packages/nux/src/components/inline-tip/index.js

+		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

packages/block-editor/package.json Show resolved Hide resolved

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

packages/nux/src/components/inline-tip/index.js Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NUX: Make tips appear inline #16582

NUX: Make tips appear inline #16582

noisysocks commented Jul 15, 2019

talldan left a comment

talldan Jul 15, 2019

kjellr commented Jul 15, 2019

noisysocks Jul 19, 2019

mapk commented Jul 31, 2019

noisysocks commented Aug 1, 2019 •

edited

Loading

noisysocks commented Aug 2, 2019 •

edited

Loading

talldan left a comment •

edited

Loading

talldan Aug 2, 2019

talldan Aug 2, 2019 •

edited

Loading

talldan Aug 2, 2019

talldan Aug 2, 2019

talldan Aug 2, 2019

mapk commented Aug 6, 2019

kjellr commented Aug 6, 2019

noisysocks commented Aug 6, 2019

kjellr commented Sep 4, 2019

NUX: Make tips appear inline #16582

NUX: Make tips appear inline #16582

Conversation

noisysocks commented Jul 15, 2019

talldan left a comment

Choose a reason for hiding this comment

talldan Jul 15, 2019

Choose a reason for hiding this comment

kjellr commented Jul 15, 2019

noisysocks Jul 19, 2019

Choose a reason for hiding this comment

mapk commented Jul 31, 2019

noisysocks commented Aug 1, 2019 • edited Loading

noisysocks commented Aug 2, 2019 • edited Loading

talldan left a comment • edited Loading

Choose a reason for hiding this comment

talldan Aug 2, 2019

Choose a reason for hiding this comment

talldan Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

talldan Aug 2, 2019

Choose a reason for hiding this comment

talldan Aug 2, 2019

Choose a reason for hiding this comment

talldan Aug 2, 2019

Choose a reason for hiding this comment

mapk commented Aug 6, 2019

kjellr commented Aug 6, 2019

noisysocks commented Aug 6, 2019

kjellr commented Sep 4, 2019

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