From 1bb5cede5da0867fddc57120cd710c6a55f85db7 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 22 Aug 2018 13:09:49 +1000 Subject: [PATCH] Revert "NUX: Prevent duplicate DotTips from appearing (#8642)" This reverts commit 09dc71c30ebcbf5b1b4475cab433eaa9b21e1908. --- .../test/__snapshots__/index.js.snap | 24 +--- .../test/__snapshots__/index.js.snap | 4 +- packages/nux/src/components/dot-tip/README.md | 4 +- packages/nux/src/components/dot-tip/index.js | 104 +++++++----------- .../nux/src/components/dot-tip/test/index.js | 8 +- packages/nux/src/store/actions.js | 37 ------- packages/nux/src/store/reducer.js | 37 +------ packages/nux/src/store/selectors.js | 31 ++---- packages/nux/src/store/test/actions.js | 29 +---- packages/nux/src/store/test/reducer.js | 79 +------------ packages/nux/src/store/test/selectors.js | 47 -------- 11 files changed, 70 insertions(+), 334 deletions(-) diff --git a/packages/editor/src/components/default-block-appender/test/__snapshots__/index.js.snap b/packages/editor/src/components/default-block-appender/test/__snapshots__/index.js.snap index 2917c9e6726564..ee4db832dc9d3f 100644 --- a/packages/editor/src/components/default-block-appender/test/__snapshots__/index.js.snap +++ b/packages/editor/src/components/default-block-appender/test/__snapshots__/index.js.snap @@ -25,13 +25,7 @@ exports[`DefaultBlockAppender should append a default block when input focused 1 - - 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! - - + /> `; @@ -53,13 +47,7 @@ exports[`DefaultBlockAppender should match snapshot 1`] = ` - - 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! - - + /> `; @@ -81,12 +69,6 @@ exports[`DefaultBlockAppender should optionally show without prompt 1`] = ` - - 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! - - + /> `; diff --git a/packages/editor/src/components/post-preview-button/test/__snapshots__/index.js.snap b/packages/editor/src/components/post-preview-button/test/__snapshots__/index.js.snap index 231157b4f2e7fb..3ede1652d63c11 100644 --- a/packages/editor/src/components/post-preview-button/test/__snapshots__/index.js.snap +++ b/packages/editor/src/components/post-preview-button/test/__snapshots__/index.js.snap @@ -8,10 +8,10 @@ exports[`PostPreviewButton render() should match the snapshot 1`] = ` onClick={[Function]} > Preview - Click “Preview” to load a preview of this page, so you can make sure you’re happy with your blocks. - + `; diff --git a/packages/nux/src/components/dot-tip/README.md b/packages/nux/src/components/dot-tip/README.md index 4fb33bb9ddae39..caf164760ffeca 100644 --- a/packages/nux/src/components/dot-tip/README.md +++ b/packages/nux/src/components/dot-tip/README.md @@ -3,8 +3,6 @@ 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 @@ -23,7 +21,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`. Changing the identifier after the component has mounted is not supported. +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`. - Type: `string` - Required: Yes diff --git a/packages/nux/src/components/dot-tip/index.js b/packages/nux/src/components/dot-tip/index.js index 80e33285906fbf..b41c794e7193bf 100644 --- a/packages/nux/src/components/dot-tip/index.js +++ b/packages/nux/src/components/dot-tip/index.js @@ -1,84 +1,64 @@ /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; -import { compose, withInstanceId } from '@wordpress/compose'; +import { compose } from '@wordpress/compose'; import { Popover, Button, IconButton } from '@wordpress/components'; import { __ } from '@wordpress/i18n'; import { withSelect, withDispatch } from '@wordpress/data'; -export class DotTip extends Component { - componentDidMount() { - this.props.onRegister(); +export function DotTip( { + children, + isVisible, + hasNextTip, + onDismiss, + onDisable, +} ) { + if ( ! isVisible ) { + return null; } - componentWillUnmount() { - this.props.onUnregister(); - } - - render() { - const { children, isVisible, hasNextTip, onDismiss, onDisable } = this.props; - - if ( ! isVisible ) { - return null; - } - - return ( - { - // 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(); - } } - > -

{ children }

-

- -

- - - ); - } + return ( + { + // 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(); + } } + > +

{ children }

+

+ +

+ + + ); } export default compose( - withInstanceId, - withSelect( ( select, { id, instanceId } ) => { + withSelect( ( select, { id } ) => { const { isTipVisible, getAssociatedGuide } = select( 'core/nux' ); const associatedGuide = getAssociatedGuide( id ); return { - isVisible: isTipVisible( id, instanceId ), + isVisible: isTipVisible( id ), hasNextTip: !! ( associatedGuide && associatedGuide.nextTipId ), }; } ), - withDispatch( ( dispatch, { id, instanceId } ) => { - const { - registerTipInstance, - unregisterTipInstance, - dismissTip, - disableTips, - } = dispatch( 'core/nux' ); - + withDispatch( ( dispatch, { id } ) => { + const { dismissTip, disableTips } = dispatch( 'core/nux' ); return { - onRegister() { - registerTipInstance( id, instanceId ); - }, - onUnregister() { - unregisterTipInstance( id, instanceId ); - }, onDismiss() { dismissTip( id ); }, diff --git a/packages/nux/src/components/dot-tip/test/index.js b/packages/nux/src/components/dot-tip/test/index.js index 884c9422ecb77c..aa69872d5e6e80 100644 --- a/packages/nux/src/components/dot-tip/test/index.js +++ b/packages/nux/src/components/dot-tip/test/index.js @@ -14,7 +14,7 @@ jest.mock( '../../../../../components/src/button' ); describe( 'DotTip', () => { it( 'should not render anything if invisible', () => { const wrapper = shallow( - + It looks like you’re writing a letter. Would you like help? ); @@ -23,7 +23,7 @@ describe( 'DotTip', () => { it( 'should render correctly', () => { const wrapper = shallow( - + It looks like you’re writing a letter. Would you like help? ); @@ -33,7 +33,7 @@ describe( 'DotTip', () => { it( 'should call onDismiss when the dismiss button is clicked', () => { const onDismiss = jest.fn(); const wrapper = shallow( - + It looks like you’re writing a letter. Would you like help? ); @@ -44,7 +44,7 @@ describe( 'DotTip', () => { it( 'should call onDisable when the X button is clicked', () => { const onDisable = jest.fn(); const wrapper = shallow( - + It looks like you’re writing a letter. Would you like help? ); diff --git a/packages/nux/src/store/actions.js b/packages/nux/src/store/actions.js index c29fbfd0ed290b..ad8adb79c5530d 100644 --- a/packages/nux/src/store/actions.js +++ b/packages/nux/src/store/actions.js @@ -13,43 +13,6 @@ export function triggerGuide( tipIds ) { }; } -/** - * Returns an action object that, when dispatched, associates an instance of the - * 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. diff --git a/packages/nux/src/store/reducer.js b/packages/nux/src/store/reducer.js index 24096aeaee8a5f..5ebf8ea394e70b 100644 --- a/packages/nux/src/store/reducer.js +++ b/packages/nux/src/store/reducer.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import { uniq, without } from 'lodash'; - /** * WordPress dependencies */ @@ -29,36 +24,6 @@ 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': { - const existingInstanceIds = state[ action.tipId ] || []; - return { - ...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. * @@ -105,4 +70,4 @@ export function dismissedTips( state = {}, action ) { const preferences = combineReducers( { areTipsEnabled, dismissedTips } ); -export default combineReducers( { guides, tipInstanceIds, preferences } ); +export default combineReducers( { guides, preferences } ); diff --git a/packages/nux/src/store/selectors.js b/packages/nux/src/store/selectors.js index 149cd30dfb0a3e..61f0193d90f46c 100644 --- a/packages/nux/src/store/selectors.js +++ b/packages/nux/src/store/selectors.js @@ -41,40 +41,29 @@ export const getAssociatedGuide = createSelector( ); /** - * Determines whether the given tip or DotTip instance should be visible. Checks: + * 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. * - * - 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. + * @param {Object} state Global application state. + * @param {string} id The tip to query. * - * @return {boolean} Whether the given tip or Dottip instance should be shown. + * @return {boolean} Whether or not the given tip is showing. */ -export function isTipVisible( state, tipId, instanceId ) { +export function isTipVisible( state, id ) { if ( ! state.preferences.areTipsEnabled ) { return false; } - if ( state.preferences.dismissedTips[ tipId ] ) { + if ( state.preferences.dismissedTips[ id ] ) { return false; } - const associatedGuide = getAssociatedGuide( state, tipId ); - if ( associatedGuide && associatedGuide.currentTipId !== tipId ) { + const associatedGuide = getAssociatedGuide( state, id ); + if ( associatedGuide && associatedGuide.currentTipId !== id ) { return false; } - if ( instanceId ) { - const [ firstInstanceId ] = state.tipInstanceIds[ tipId ] || []; - if ( instanceId !== firstInstanceId ) { - return false; - } - } - return true; } diff --git a/packages/nux/src/store/test/actions.js b/packages/nux/src/store/test/actions.js index 7bdf580fe9eb83..4e22afe03c8b82 100644 --- a/packages/nux/src/store/test/actions.js +++ b/packages/nux/src/store/test/actions.js @@ -1,14 +1,7 @@ /** * Internal dependencies */ -import { - triggerGuide, - registerTipInstance, - unregisterTipInstance, - dismissTip, - disableTips, - enableTips, -} from '../actions'; +import { triggerGuide, dismissTip, disableTips, enableTips } from '../actions'; describe( 'actions', () => { describe( 'triggerGuide', () => { @@ -20,26 +13,6 @@ describe( 'actions', () => { } ); } ); - describe( 'registerTipInstance', () => { - it( 'should return a REGISTER_TIP_INSTANCE action', () => { - expect( registerTipInstance( 'test/tip-1', 123 ) ).toEqual( { - type: 'REGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 123, - } ); - } ); - } ); - - describe( 'unregisterTipInstance', () => { - it( 'should return an UNREGISTER_TIP_INSTANCE action', () => { - expect( unregisterTipInstance( 'test/tip-1', 123 ) ).toEqual( { - type: 'UNREGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 123, - } ); - } ); - } ); - describe( 'dismissTip', () => { it( 'should return an DISMISS_TIP action', () => { expect( dismissTip( 'test/tip' ) ).toEqual( { diff --git a/packages/nux/src/store/test/reducer.js b/packages/nux/src/store/test/reducer.js index 81197e83b0942d..d2982a87d70501 100644 --- a/packages/nux/src/store/test/reducer.js +++ b/packages/nux/src/store/test/reducer.js @@ -1,12 +1,7 @@ -/** - * External dependencies - */ -import deepFreeze from 'deep-freeze'; - /** * Internal dependencies */ -import { guides, tipInstanceIds, areTipsEnabled, dismissedTips } from '../reducer'; +import { guides, areTipsEnabled, dismissedTips } from '../reducer'; describe( 'reducer', () => { describe( 'guides', () => { @@ -15,8 +10,7 @@ describe( 'reducer', () => { } ); it( 'should add a guide when it is triggered', () => { - const original = deepFreeze( [] ); - const state = guides( original, { + const state = guides( [], { type: 'TRIGGER_GUIDE', tipIds: [ 'test/tip-1', 'test/tip-2' ], } ); @@ -26,66 +20,6 @@ describe( 'reducer', () => { } ); } ); - describe( 'tipInstanceIds', () => { - it( 'should start out empty', () => { - expect( tipInstanceIds( undefined, {} ) ).toEqual( {} ); - } ); - - it( 'should register an initial tip instance', () => { - const original = deepFreeze( {} ); - const state = tipInstanceIds( original, { - type: 'REGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 123, - } ); - expect( state ).toEqual( { - 'test/tip-1': [ 123 ], - } ); - } ); - - it( 'should register an second tip instance', () => { - const original = deepFreeze( { - 'test/tip-1': [ 123 ], - } ); - const state = tipInstanceIds( original, { - type: 'REGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 456, - } ); - expect( state ).toEqual( { - 'test/tip-1': [ 123, 456 ], - } ); - } ); - - it( 'should not register duplicate tip instances', () => { - const original = deepFreeze( { - 'test/tip-1': [ 123 ], - } ); - const state = tipInstanceIds( original, { - type: 'REGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 123, - } ); - expect( state ).toEqual( { - 'test/tip-1': [ 123 ], - } ); - } ); - - it( 'should unregister a tip instance', () => { - const original = deepFreeze( { - 'test/tip-1': [ 123, 456 ], - } ); - const state = tipInstanceIds( original, { - type: 'UNREGISTER_TIP_INSTANCE', - tipId: 'test/tip-1', - instanceId: 123, - } ); - expect( state ).toEqual( { - 'test/tip-1': [ 456 ], - } ); - } ); - } ); - describe( 'areTipsEnabled', () => { it( 'should default to true', () => { expect( areTipsEnabled( undefined, {} ) ).toBe( true ); @@ -112,8 +46,7 @@ describe( 'reducer', () => { } ); it( 'should mark tips as dismissed', () => { - const original = deepFreeze( {} ); - const state = dismissedTips( original, { + const state = dismissedTips( {}, { type: 'DISMISS_TIP', id: 'test/tip', } ); @@ -123,10 +56,10 @@ describe( 'reducer', () => { } ); it( 'should reset if tips are enabled', () => { - const original = deepFreeze( { + const initialState = { 'test/tip': true, - } ); - const state = dismissedTips( original, { + }; + const state = dismissedTips( initialState, { type: 'ENABLE_TIPS', } ); expect( state ).toEqual( {} ); diff --git a/packages/nux/src/store/test/selectors.js b/packages/nux/src/store/test/selectors.js index f807957abe46d5..ed63a7fa828de5 100644 --- a/packages/nux/src/store/test/selectors.js +++ b/packages/nux/src/store/test/selectors.js @@ -11,7 +11,6 @@ describe( 'selectors', () => { [ 'test/tip-a', 'test/tip-b', 'test/tip-c' ], [ 'test/tip-α', 'test/tip-β', 'test/tip-γ' ], ], - tipInstanceIds: {}, preferences: { dismissedTips: { 'test/tip-1': true, @@ -57,7 +56,6 @@ describe( 'selectors', () => { it( 'should return true by default', () => { const state = { guides: [], - tipInstanceIds: {}, preferences: { areTipsEnabled: true, dismissedTips: {}, @@ -69,7 +67,6 @@ describe( 'selectors', () => { it( 'should return false if tips are disabled', () => { const state = { guides: [], - tipInstanceIds: {}, preferences: { areTipsEnabled: false, dismissedTips: {}, @@ -81,7 +78,6 @@ describe( 'selectors', () => { it( 'should return false if the tip is dismissed', () => { const state = { guides: [], - tipInstanceIds: {}, preferences: { areTipsEnabled: true, dismissedTips: { @@ -97,7 +93,6 @@ describe( 'selectors', () => { guides: [ [ 'test/tip-1', 'test/tip-2', 'test/tip-3' ], ], - tipInstanceIds: {}, preferences: { areTipsEnabled: true, dismissedTips: {}, @@ -105,53 +100,12 @@ describe( 'selectors', () => { }; expect( isTipVisible( state, 'test/tip-2' ) ).toBe( false ); } ); - - it( 'should return true if an instanceId that was registered first is provided', () => { - const state = { - guides: [], - tipInstanceIds: { - 'test/tip': [ 123, 456 ], - }, - preferences: { - areTipsEnabled: true, - dismissedTips: {}, - }, - }; - expect( isTipVisible( state, 'test/tip', 123 ) ).toBe( true ); - } ); - - it( 'should return false if an instanceId that was NOT registered is provided', () => { - const state = { - guides: [], - tipInstanceIds: {}, - preferences: { - areTipsEnabled: true, - dismissedTips: {}, - }, - }; - expect( isTipVisible( state, 'test/tip', 123 ) ).toBe( false ); - } ); - - it( 'should return false if an instanceId that was NOT registered first is provided', () => { - const state = { - guides: [], - tipInstanceIds: { - 'test/tip': [ 123, 456 ], - }, - preferences: { - areTipsEnabled: true, - dismissedTips: {}, - }, - }; - expect( isTipVisible( state, 'test/tip', 456 ) ).toBe( false ); - } ); } ); describe( 'areTipsEnabled', () => { it( 'should return true if tips are enabled', () => { const state = { guides: [], - tipInstanceIds: {}, preferences: { areTipsEnabled: true, dismissedTips: {}, @@ -163,7 +117,6 @@ describe( 'selectors', () => { it( 'should return false if tips are disabled', () => { const state = { guides: [], - tipInstanceIds: {}, preferences: { areTipsEnabled: false, dismissedTips: {},