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: BlockListAppender: 1.7x increase on key press #12510

Merged
merged 1 commit into from
Dec 9, 2018
Merged

Performance: BlockListAppender: 1.7x increase on key press #12510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 11 additions & 3 deletions packages/editor/src/components/default-block-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import TextareaAutosize from 'react-autosize-textarea';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { compose } from '@wordpress/compose';
import { compose, withState } from '@wordpress/compose';
import { getDefaultBlockName } from '@wordpress/blocks';
import { decodeEntities } from '@wordpress/html-entities';
import { withSelect, withDispatch } from '@wordpress/data';
Expand All @@ -26,6 +26,8 @@ export function DefaultBlockAppender( {
showPrompt,
placeholder,
rootClientId,
hovered,
setState,
} ) {
if ( isLocked || ! isVisible ) {
return null;
Expand All @@ -49,7 +51,12 @@ export function DefaultBlockAppender( {
// The wp-block className is important for editor styles.

return (
<div data-root-client-id={ rootClientId || '' } className="wp-block editor-default-block-appender">
<div
data-root-client-id={ rootClientId || '' }
className="wp-block editor-default-block-appender"
onMouseEnter={ () => setState( { hovered: true } ) }
Copy link
Member

Choose a reason for hiding this comment

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

Since we're in the realm of performance optimizations, this being a consistent reference would avoid unnecessary React reconciliation. Easiest with a class, e.g. onMouseEnter={ this.onMouseEnter }

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the point of withState then?

Copy link
Member

@aduth aduth Dec 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There's nothing wrong with the usage here. What I'm suggesting is a micro-optimization which is more often okay to concede for the DevEx gains offered by withState.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, I get the optimisation, but it seems odd to me that withState even exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be cool if you could add functions to withState that are then passed as props?

Copy link
Member

Choose a reason for hiding this comment

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

withState's original purpose was more oriented toward block implementers, to keep them in the realm of more-readily-understood function components, where state was the most common factor in "upgrading" to the component class.

Copy link
Member

Choose a reason for hiding this comment

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

Would be cool if you could add functions to withState that are then passed as props?

Yeah, I think there's some room for improvement here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Block implementers' components might also show when the user is typing, so it'd be good to try to improve withState. I'll have a look.

onMouseLeave={ () => setState( { hovered: false } ) }
>
<BlockDropZone rootClientId={ rootClientId } />
<TextareaAutosize
role="button"
Expand All @@ -59,12 +66,13 @@ export function DefaultBlockAppender( {
onFocus={ onAppend }
value={ showPrompt ? value : '' }
/>
<InserterWithShortcuts rootClientId={ rootClientId } />
{ hovered && <InserterWithShortcuts rootClientId={ rootClientId } /> }
<Inserter position="top right" />
</div>
);
}
export default compose(
withState( { hovered: false } ),
withSelect( ( select, ownProps ) => {
const { getBlockCount, getBlockName, isBlockValid, getEditorSettings, getTemplateLock } = select( 'core/editor' );

Expand Down
9 changes: 6 additions & 3 deletions packages/editor/src/components/default-block-appender/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ exports[`DefaultBlockAppender should append a default block when input focused 1
<div
className="wp-block editor-default-block-appender"
data-root-client-id=""
onMouseEnter={[Function]}
onMouseLeave={[Function]}
>
<WithDispatch(WithSelect(WithFilters(BlockDropZone))) />
<TextareaAutosize
Expand All @@ -22,7 +24,6 @@ exports[`DefaultBlockAppender should append a default block when input focused 1
rows={1}
value="Start writing or type / to choose a block"
/>
<WithSelect(WithDispatch(InserterWithShortcuts)) />
<WithSelect(IfCondition(Inserter))
position="top right"
/>
Expand All @@ -33,6 +34,8 @@ exports[`DefaultBlockAppender should match snapshot 1`] = `
<div
className="wp-block editor-default-block-appender"
data-root-client-id=""
onMouseEnter={[Function]}
onMouseLeave={[Function]}
>
<WithDispatch(WithSelect(WithFilters(BlockDropZone))) />
<TextareaAutosize
Expand All @@ -44,7 +47,6 @@ exports[`DefaultBlockAppender should match snapshot 1`] = `
rows={1}
value="Start writing or type / to choose a block"
/>
<WithSelect(WithDispatch(InserterWithShortcuts)) />
<WithSelect(IfCondition(Inserter))
position="top right"
/>
Expand All @@ -55,6 +57,8 @@ exports[`DefaultBlockAppender should optionally show without prompt 1`] = `
<div
className="wp-block editor-default-block-appender"
data-root-client-id=""
onMouseEnter={[Function]}
onMouseLeave={[Function]}
>
<WithDispatch(WithSelect(WithFilters(BlockDropZone))) />
<TextareaAutosize
Expand All @@ -66,7 +70,6 @@ exports[`DefaultBlockAppender should optionally show without prompt 1`] = `
rows={1}
value=""
/>
<WithSelect(WithDispatch(InserterWithShortcuts)) />
<WithSelect(IfCondition(Inserter))
position="top right"
/>
Expand Down