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

Save post button: avoid extra re-renders when enablng/disabling tooltip #56502

Merged
merged 5 commits into from
Nov 25, 2023
Merged
Changes from 4 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
83 changes: 46 additions & 37 deletions packages/editor/src/components/post-saved-state/index.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewing the diff with the "Hide whitespace" option enabled can be quite helpful here

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import classnames from 'classnames';
import {
__unstableGetAnimateClassName as getAnimateClassName,
Button,
Tooltip,
} from '@wordpress/components';
import { usePrevious, useViewportMatch } from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';
Expand Down Expand Up @@ -128,45 +129,53 @@ export default function PostSavedState( {
text = shortLabel;
}

const buttonAccessibleLabel = text || label;

/**
* The tooltip needs to be enabled only if the button is not disabled. When
* relying on the internal Button tooltip functionality, this causes the
* resulting `button` element to be always removed and re-added to the DOM,
* causing focus loss. An alternative approach to circumvent the issue
* is not to use the `label` and `shortcut` props on `Button` (which would
* trigger the tooltip), and instead manually wrap the `Button` in a separate
* `Tooltip` component.
*/
const tooltipProps = isDisabled
? undefined
: {
text: buttonAccessibleLabel,
shortcut: displayShortcut.primary( 's' ),
};

// Use common Button instance for all saved states so that focus is not
// lost.
return (
<Button
className={
isSaveable || isSaving
? classnames( {
'editor-post-save-draft': ! isSavedState,
'editor-post-saved-state': isSavedState,
'is-saving': isSaving,
'is-autosaving': isAutosaving,
'is-saved': isSaved,
[ getAnimateClassName( {
type: 'loading',
} ) ]: isSaving,
} )
: undefined
}
onClick={ isDisabled ? undefined : () => savePost() }
/*
* We want the tooltip to show the keyboard shortcut only when the
* button does something, i.e. when it's not disabled.
*/
shortcut={ isDisabled ? undefined : displayShortcut.primary( 's' ) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prop is now passed to the Tooltip component instead

/*
* Displaying the keyboard shortcut conditionally makes the tooltip
* itself show conditionally. This would trigger a full-rerendering
* of the button that we want to avoid. By setting `showTooltip`,
* the tooltip is always rendered even when there's no keyboard shortcut.
*/
showTooltip
variant="tertiary"
icon={ isLargeViewport ? undefined : cloudUpload }
// Make sure the aria-label has always a value, as the default `text` is undefined on small screens.
label={ text || label }
Copy link
Contributor Author

@ciampo ciampo Nov 24, 2023

Choose a reason for hiding this comment

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

the label prop has two functions in button:

  • it serves as an aria-label, if the aria-label prop isn't explicitly set
  • it's used as tooltip text

Swapping label for aria-label allows us to keep the button correctly labelled, without triggering the internal Button's tooltip.

The same string of text is also being passed explicitly to the Tooltip component via the text prop.

aria-disabled={ isDisabled }
>
{ isSavedState && <Icon icon={ isSaved ? check : cloud } /> }
{ text }
</Button>
<Tooltip { ...tooltipProps }>
<Button
className={
isSaveable || isSaving
? classnames( {
'editor-post-save-draft': ! isSavedState,
'editor-post-saved-state': isSavedState,
'is-saving': isSaving,
'is-autosaving': isAutosaving,
'is-saved': isSaved,
[ getAnimateClassName( {
type: 'loading',
} ) ]: isSaving,
} )
: undefined
}
onClick={ isDisabled ? undefined : () => savePost() }
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed we can shorten this to:

Suggested change
onClick={ isDisabled ? undefined : () => savePost() }
onClick={ isDisabled ? undefined : savePost }

variant="tertiary"
icon={ isLargeViewport ? undefined : cloudUpload }
// Make sure the aria-label has always a value, as the default `text` is undefined on small screens.
aria-label={ buttonAccessibleLabel }
aria-disabled={ isDisabled }
>
{ isSavedState && <Icon icon={ isSaved ? check : cloud } /> }
{ text }
</Button>
</Tooltip>
);
}
Loading