-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { union, without } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
|
@@ -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 ] ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to handle the case where 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. | ||
* | ||
|
@@ -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 } ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ] || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There might be another use case that I'm missing, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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