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: Prevent duplicate DotTips from appearing #8642

Merged
merged 1 commit into from
Aug 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ exports[`DefaultBlockAppender should append a default block when input focused 1
<WithSelect(WithDispatch(Inserter))
position="top right"
>
<WithSelect(WithDispatch(DotTip))
<WithInstanceId(WithSelect(WithDispatch(DotTip)))
id="core/editor.inserter"
>
Welcome to the wonderful world of blocks! Click the “+” (“Add block”) button to add a new block. There are blocks available for all kind of content: you can insert text, headings, images, lists, and lots more!
</WithSelect(WithDispatch(DotTip))>
</WithInstanceId(WithSelect(WithDispatch(DotTip)))>
</WithSelect(WithDispatch(Inserter))>
</div>
`;
Expand All @@ -72,11 +72,11 @@ exports[`DefaultBlockAppender should match snapshot 1`] = `
<WithSelect(WithDispatch(Inserter))
position="top right"
>
<WithSelect(WithDispatch(DotTip))
<WithInstanceId(WithSelect(WithDispatch(DotTip)))
id="core/editor.inserter"
>
Welcome to the wonderful world of blocks! Click the “+” (“Add block”) button to add a new block. There are blocks available for all kind of content: you can insert text, headings, images, lists, and lots more!
</WithSelect(WithDispatch(DotTip))>
</WithInstanceId(WithSelect(WithDispatch(DotTip)))>
</WithSelect(WithDispatch(Inserter))>
</div>
`;
Expand All @@ -102,11 +102,11 @@ exports[`DefaultBlockAppender should optionally show without prompt 1`] = `
<WithSelect(WithDispatch(Inserter))
position="top right"
>
<WithSelect(WithDispatch(DotTip))
<WithInstanceId(WithSelect(WithDispatch(DotTip)))
id="core/editor.inserter"
>
Welcome to the wonderful world of blocks! Click the “+” (“Add block”) button to add a new block. There are blocks available for all kind of content: you can insert text, headings, images, lists, and lots more!
</WithSelect(WithDispatch(DotTip))>
</WithInstanceId(WithSelect(WithDispatch(DotTip)))>
</WithSelect(WithDispatch(Inserter))>
</div>
`;
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ exports[`PostPreviewButton render() should match the snapshot 1`] = `
onClick={[Function]}
>
Preview
<WithSelect(WithDispatch(DotTip))
<WithInstanceId(WithSelect(WithDispatch(DotTip)))
id="core/editor.preview"
>
Click ‘Preview’ to load a preview of this page, so you can make sure you’re happy with your blocks.
</WithSelect(WithDispatch(DotTip))>
</WithInstanceId(WithSelect(WithDispatch(DotTip)))>
</Button>
`;
4 changes: 3 additions & 1 deletion packages/nux/src/components/dot-tip/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ DotTip

`DotTip` is a React component that renders a single _tip_ on the screen. The tip will point to the React element that `DotTip` is nested within. Each tip is uniquely identified by a string passed to `id`.

When there are multiple instances of `DotTip` that reference the same tip identifier, only the first instance to have been mounted will be visible.

## Usage

```jsx
Expand All @@ -21,7 +23,7 @@ The component accepts the following props:

### id

A string that uniquely identifies the tip. Identifiers should be prefixed with the name of the plugin, followed by a `/`. For example, `acme/add-to-cart`.
A string that uniquely identifies the tip. Identifiers should be prefixed with the name of the plugin, followed by a `/`. For example, `acme/add-to-cart`. Changing the identifier after the component has mounted is not supported.

- Type: `string`
- Required: Yes
Expand Down
104 changes: 62 additions & 42 deletions packages/nux/src/components/dot-tip/index.js
Original file line number Diff line number Diff line change
@@ -1,64 +1,84 @@
/**
* WordPress dependencies
*/
import { compose } from '@wordpress/compose';
import { Component } from '@wordpress/element';
import { compose, withInstanceId } from '@wordpress/compose';
import { Popover, Button, IconButton } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { withSelect, withDispatch } from '@wordpress/data';

export function DotTip( {
children,
isVisible,
hasNextTip,
onDismiss,
onDisable,
} ) {
if ( ! isVisible ) {
return null;
export class DotTip extends Component {
componentDidMount() {
this.props.onRegister();
}

return (
<Popover
className="nux-dot-tip"
position="middle right"
noArrow
focusOnMount="container"
role="dialog"
aria-label={ __( 'Gutenberg tips' ) }
onClick={ ( event ) => {
// Tips are often nested within buttons. We stop propagation so that clicking
// on a tip doesn't result in the button being clicked.
event.stopPropagation();
} }
>
<p>{ children }</p>
<p>
<Button isLink onClick={ onDismiss }>
{ hasNextTip ? __( 'See next tip' ) : __( 'Got it' ) }
</Button>
</p>
<IconButton
className="nux-dot-tip__disable"
icon="no-alt"
label={ __( 'Disable tips' ) }
onClick={ onDisable }
/>
</Popover>
);
componentWillUnmount() {
this.props.onUnregister();
}

render() {
const { children, isVisible, hasNextTip, onDismiss, onDisable } = this.props;

if ( ! isVisible ) {
return null;
}

return (
<Popover
className="nux-dot-tip"
position="middle right"
noArrow
focusOnMount="container"
role="dialog"
aria-label={ __( 'Gutenberg tips' ) }
onClick={ ( event ) => {
// Tips are often nested within buttons. We stop propagation so that clicking
// on a tip doesn't result in the button being clicked.
event.stopPropagation();
} }
>
<p>{ children }</p>
<p>
<Button isLink onClick={ onDismiss }>
{ hasNextTip ? __( 'See next tip' ) : __( 'Got it' ) }
</Button>
</p>
<IconButton
className="nux-dot-tip__disable"
icon="no-alt"
label={ __( 'Disable tips' ) }
onClick={ onDisable }
/>
</Popover>
);
}
}

export default compose(
withSelect( ( select, { id } ) => {
withInstanceId,
withSelect( ( select, { id, instanceId } ) => {
const { isTipVisible, getAssociatedGuide } = select( 'core/nux' );
const associatedGuide = getAssociatedGuide( id );
return {
isVisible: isTipVisible( id ),
isVisible: isTipVisible( id, instanceId ),
hasNextTip: !! ( associatedGuide && associatedGuide.nextTipId ),
};
} ),
withDispatch( ( dispatch, { id } ) => {
const { dismissTip, disableTips } = dispatch( 'core/nux' );
withDispatch( ( dispatch, { id, instanceId } ) => {
const {
registerTipInstance,
unregisterTipInstance,
dismissTip,
disableTips,
} = dispatch( 'core/nux' );

return {
onRegister() {
registerTipInstance( id, instanceId );
},
onUnregister() {
unregisterTipInstance( id, instanceId );
},
onDismiss() {
dismissTip( id );
},
Expand Down
8 changes: 4 additions & 4 deletions packages/nux/src/components/dot-tip/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jest.mock( '../../../../../components/src/button' );
describe( 'DotTip', () => {
it( 'should not render anything if invisible', () => {
const wrapper = shallow(
<DotTip>
<DotTip onRegister={ noop } onUnregister={ noop }>
It looks like you’re writing a letter. Would you like help?
</DotTip>
);
Expand All @@ -23,7 +23,7 @@ describe( 'DotTip', () => {

it( 'should render correctly', () => {
const wrapper = shallow(
<DotTip isVisible setTimeout={ noop }>
<DotTip isVisible onRegister={ noop } onUnregister={ noop }>
It looks like you’re writing a letter. Would you like help?
</DotTip>
);
Expand All @@ -33,7 +33,7 @@ describe( 'DotTip', () => {
it( 'should call onDismiss when the dismiss button is clicked', () => {
const onDismiss = jest.fn();
const wrapper = shallow(
<DotTip isVisible onDismiss={ onDismiss } setTimeout={ noop }>
<DotTip isVisible onDismiss={ onDismiss } onRegister={ noop } onUnregister={ noop }>
It looks like you’re writing a letter. Would you like help?
</DotTip>
);
Expand All @@ -44,7 +44,7 @@ describe( 'DotTip', () => {
it( 'should call onDisable when the X button is clicked', () => {
const onDisable = jest.fn();
const wrapper = shallow(
<DotTip isVisible onDisable={ onDisable } setTimeout={ noop }>
<DotTip isVisible onDisable={ onDisable } onRegister={ noop } onUnregister={ noop }>
It looks like you’re writing a letter. Would you like help?
</DotTip>
);
Expand Down
37 changes: 37 additions & 0 deletions packages/nux/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,43 @@ export function triggerGuide( tipIds ) {
};
}

/**
* Returns an action object that, when dispathed, associates an instance of the
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typo here - dispathed -> dispatched

* DotTip component with a tip. This is usually done when the component mounts.
* Tracking this lets us only show one DotTip at a time per tip.
*
* @param {string} tipId The tip to associate this instance with.
* @param {number} instanceId A number which uniquely identifies the instance.
*
* @return {Object} Action object.
*/
export function registerTipInstance( tipId, instanceId ) {
return {
type: 'REGISTER_TIP_INSTANCE',
tipId,
instanceId,
};
}

/**
* Returns an action object that, when dispatched, removes the association
* between a DotTip component instance and a tip. This is usually done when the
* component unmounts. Tracking this lets us only show one DotTip at a time per
* tip.
*
* @param {string} tipId The tip to disassociate this instance with.
* @param {number} instanceId A number which uniquely identifies the instance.
*
* @return {Object} Action object.
*/
export function unregisterTipInstance( tipId, instanceId ) {
return {
type: 'UNREGISTER_TIP_INSTANCE',
tipId,
instanceId,
};
}

/**
* Returns an action object that, when dispatched, dismisses the given tip. A
* dismissed tip will not show again.
Expand Down
35 changes: 34 additions & 1 deletion packages/nux/src/store/reducer.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { union, without } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -24,6 +29,34 @@ export function guides( state = [], action ) {
return state;
}

/**
* Reducer that maps each tip to an array of DotTip component instance IDs that are
* displaying that tip. Tracking this allows us to only show one DotTip at a
* time per tip.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function tipInstanceIds( state = {}, action ) {
switch ( action.type ) {
case 'REGISTER_TIP_INSTANCE':
return {
...state,
[ action.tipId ]: union( state[ action.tipId ] || [], [ action.instanceId ] ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Found this quite a hard line to read initially. I wonder if this would be more succinct? I think they do the same thing.

uniq( [ ...state[ action.tipId ], action.instanceId ] )

Copy link
Member Author

@noisysocks noisysocks Aug 9, 2018

Choose a reason for hiding this comment

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

We need to handle the case where state[ action.tipId ] is undefined.

uniq( [ ...state[ action.tipId ] || [], action.instanceId ] )

Maybe I'll just use a temporary variable.

const existingInstanceIds = state[ action.tipId ] || [];
uniq( [ ...existingInstanceIds, action.instanceId ] )

};

case 'UNREGISTER_TIP_INSTANCE':
return {
...state,
[ action.tipId ]: without( state[ action.tipId ], action.instanceId ),
};
}

return state;
}

/**
* Reducer that tracks whether or not tips are globally enabled.
*
Expand Down Expand Up @@ -70,4 +103,4 @@ export function dismissedTips( state = {}, action ) {

const preferences = combineReducers( { areTipsEnabled, dismissedTips } );

export default combineReducers( { guides, preferences } );
export default combineReducers( { guides, tipInstanceIds, preferences } );
31 changes: 21 additions & 10 deletions packages/nux/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,29 +41,40 @@ export const getAssociatedGuide = createSelector(
);

/**
* Determines whether or not the given tip is showing. Tips are hidden if they
* are disabled, have been dismissed, or are not the current tip in any
* guide that they have been added to.
* Determines whether the given tip or DotTip instance should be visible. Checks:
*
* @param {Object} state Global application state.
* @param {string} id The tip to query.
* - That all tips are enabled.
* - That the given tip has not been dismissed.
* - If the given tip is part of a guide, that the given tip is the current tip in the guide.
* - If instanceId is provided, that this is the first DotTip instance for the given tip.
*
* @param {Object} state Global application state.
* @param {string} tipId The tip to query.
* @param {?number} instanceId A number which uniquely identifies the DotTip instance.
*
* @return {boolean} Whether or not the given tip is showing.
* @return {boolean} Whether the given tip or Dottip instance should be shown.
*/
export function isTipVisible( state, id ) {
export function isTipVisible( state, tipId, instanceId ) {
if ( ! state.preferences.areTipsEnabled ) {
return false;
}

if ( state.preferences.dismissedTips[ id ] ) {
if ( state.preferences.dismissedTips[ tipId ] ) {
return false;
}

const associatedGuide = getAssociatedGuide( state, id );
if ( associatedGuide && associatedGuide.currentTipId !== id ) {
const associatedGuide = getAssociatedGuide( state, tipId );
if ( associatedGuide && associatedGuide.currentTipId !== tipId ) {
return false;
}

if ( instanceId ) {
const [ firstInstanceId ] = state.tipInstanceIds[ tipId ] || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering - if we only use the first instance Id, do the others need to be stored in tipInstanceIds? Potentially the value of tipInstanceIds[ tipId ] could just be the instance Id of the first instance of that tip, instead of an array of all the tips. Maybe renamed to tipActiveInstanceId or something

There might be another use case that I'm missing, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that it doesn't handle the case where the first/active instance is unmounted because of e.g. a block removal. When this happens, the tip should move to the next available instance. You actually encounter this in the reproduction steps for the bug above, because the first/active DotTip is unmounted when you insert the Columns block.

It's a pretty bizarre use-case, I'll admit. Hoping that we can make this all unnecessary either through #8050 or by otherwise iterating on these tips. Tammie and I have been working through some ideas on what to do with NUX...

if ( instanceId !== firstInstanceId ) {
return false;
}
}

return true;
}

Expand Down
Loading