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.

Sign up for GitHub

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

Performance: Remove is-typing root class #32567

Merged
merged 4 commits into from
Jun 11, 2021
Merged

Performance: Remove is-typing root class #32567

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7069153
see if removing root class has any timing differences
gwwar Jun 9, 2021
b62f8b4
Try :focus-within instead of is-typing
gwwar Jun 10, 2021
a005470
try adding a class to block when typing
gwwar Jun 10, 2021
71d7488
move remove-outline to use-block-class-names
gwwar Jun 10, 2021
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
20 changes: 18 additions & 2 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ import {
hasBlockSupport,
} from '@wordpress/blocks';
import { withFilters } from '@wordpress/components';
import { withDispatch, withSelect, useDispatch } from '@wordpress/data';
import {
withDispatch,
withSelect,
useDispatch,
useSelect,
} from '@wordpress/data';
import { compose, pure, ifCondition } from '@wordpress/compose';

/**
Expand Down Expand Up @@ -83,6 +88,15 @@ function BlockListBlock( {
const { removeBlock } = useDispatch( blockEditorStore );
const onRemove = useCallback( () => removeBlock( clientId ), [ clientId ] );

const removeBlockOutline = useSelect(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should move this to useBlockProps instead of here (the leaf place where the classname is applied)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch, useBlockClassNames felt like a better fit, so I moved that in 71d7488

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 the class could maybe use a better name, but I'd like to decouple it from is-typing so we can apply it only the situations we need it for.

( select ) => {
const { isTyping, getSettings } = select( blockEditorStore );
const isOutlineMode = getSettings().outlineMode;
return isOutlineMode && isSelected && isTyping();
},
[ clientId, isSelected ]
);

// We wrap the BlockEdit component in a div that hides it when editing in
// HTML mode. This allows us to render all of the ancillary pieces
// (InspectorControls, etc.) which are inside `BlockEdit` but not
Expand Down Expand Up @@ -159,7 +173,9 @@ function BlockListBlock( {

const value = {
clientId,
className,
className: removeBlockOutline
? classnames( className, 'remove-outline' )
: className,
wrapperProps: omit( wrapperProps, [ 'data-align' ] ),
isAligned,
};
Expand Down
34 changes: 14 additions & 20 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,20 @@ export const IntersectionObserver = createContext();

function Root( { className, children } ) {
const isLargeViewport = useViewportMatch( 'medium' );
const {
isTyping,
isOutlineMode,
isFocusMode,
isNavigationMode,
} = useSelect( ( select ) => {
const {
isTyping: _isTyping,
getSettings,
isNavigationMode: _isNavigationMode,
} = select( blockEditorStore );
const { outlineMode, focusMode } = getSettings();
return {
isTyping: _isTyping(),
isOutlineMode: outlineMode,
isFocusMode: focusMode,
isNavigationMode: _isNavigationMode(),
};
}, [] );
const { isOutlineMode, isFocusMode, isNavigationMode } = useSelect(
( select ) => {
const { getSettings, isNavigationMode: _isNavigationMode } = select(
blockEditorStore
);
const { outlineMode, focusMode } = getSettings();
return {
isOutlineMode: outlineMode,
isFocusMode: focusMode,
isNavigationMode: _isNavigationMode(),
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, it seems really annoying that all blocks have to re-render if we simply want to toggle any of these modes above and add a class. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 though hopefully these have less impact since the modes should be toggled less often 🤞

};
},
[]
);
return (
<div
ref={ useMergeRefs( [
Expand All @@ -57,7 +52,6 @@ function Root( { className, children } ) {
'block-editor-block-list__layout is-root-container',
className,
{
'is-typing': isTyping,
'is-outline-mode': isOutlineMode,
'is-focus-mode': isFocusMode && isLargeViewport,
'is-navigate-mode': isNavigationMode,
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@
}
}

.is-outline-mode:not(.is-typing) .block-editor-block-list__block {
.is-outline-mode .block-editor-block-list__block:not(.remove-outline) {
&.is-hovered {
cursor: default;

Expand Down