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

NUX: Make tips appear inline #16582

Closed
wants to merge 7 commits into from

Conversation

@noisysocks
Copy link
Member

commented Jul 15, 2019

Implements part of #16315.

localhost_8888_wp-admin_post-new php

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

To test:

  1. Create a new post
  2. Click More tools & options
  3. Select Options
  4. Check Enable Tips

@noisysocks noisysocks force-pushed the add/inline-tips branch from 076a1e1 to 2011a87 Jul 15, 2019

@talldan
Copy link
Contributor

left a comment

Did a review 👍

packages/block-editor/package.json Show resolved Hide resolved
// Options modal, which looks totally weird.
DeferredOption
);
)( BaseOption );

This comment has been minimized.

Copy link
@talldan

talldan Jul 15, 2019

Contributor

🎉

packages/nux/src/components/inline-tip/test/index.js Outdated Show resolved Hide resolved
packages/nux/src/components/inline-tip/index.js Outdated Show resolved Hide resolved
packages/nux/src/components/inline-tip/index.js Outdated Show resolved Hide resolved
packages/nux/src/components/inline-tip/README.md Outdated Show resolved Hide resolved
packages/nux/src/components/inline-tip/index.js Outdated Show resolved Hide resolved
@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

This is looking great so far!

Screen Shot 2019-07-15 at 8 20 59 AM

Screen Shot 2019-07-15 at 8 20 48 AM

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

@@ -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.

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jul 19, 2019

Author Member

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

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

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

  1. When closing a tip, I expected the message to appear inline like this: #16315 (comment) But maybe we're not going this route? Or not for version 1?

  2. 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?

tips

  1. 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"?

Screen Shot 2019-07-31 at 7 56 42 AM

@noisysocks

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@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! 🙂

@noisysocks noisysocks force-pushed the add/inline-tips branch from 48652f9 to 74e3205 Aug 2, 2019

@noisysocks noisysocks force-pushed the add/inline-tips branch from 74e3205 to 7333838 Aug 2, 2019

@noisysocks noisysocks requested a review from mkaz as a code owner Aug 2, 2019

@noisysocks

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

This should be ready for a final review! 🙏

inline tips

@talldan
Copy link
Contributor

left a comment

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.
    focus-issue

  • 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:
    menu-close

  • 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):
    focus-return

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.

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

This comment has been minimized.

Copy link
@talldan

talldan Aug 2, 2019

Contributor

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,
>
<div
className="components-notice__content"
<Fragment>

This comment has been minimized.

Copy link
@talldan

talldan Aug 2, 2019

Contributor

This snapshot now seems a bit pointless!

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

This comment has been minimized.

Copy link
@talldan

talldan Aug 2, 2019

Contributor

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

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

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

This comment has been minimized.

Copy link
@talldan

talldan Aug 2, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@talldan

talldan Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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.

Screen Shot 2019-08-05 at 6 15 44 PM

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

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

@noisysocks noisysocks closed this Aug 23, 2019

@noisysocks noisysocks deleted the add/inline-tips branch Aug 23, 2019

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

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

  1. Remove the current NUX tips.
  2. Implement the new welcome modal described in that comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.