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

Refactor the BlockToolbar compoonent to avoid getSelectedBlock #11843

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

youknowriad
Copy link
Contributor

Extracted from #11811

I'm using getSelectedBlockClientId and a new isBlockValid selector to avoid the less performant getSelectedBlock selector in the BlockToolbar.

@youknowriad youknowriad requested a review from a team November 14, 2018 08:29
@youknowriad youknowriad self-assigned this Nov 14, 2018
@youknowriad youknowriad added the [Type] Performance Related to performance efforts label Nov 14, 2018
@youknowriad youknowriad added this to the 4.4 milestone Nov 14, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I did a set of smoke tests and things looked good. The changes also seem logical 👍
There is only one small change that I think we should do before the merge.

*/
export function isBlockValid( state, clientId ) {
const block = state.editor.present.blocks.byClientId[ clientId ];
return block && block.isValid;
Copy link
Member

Choose a reason for hiding this comment

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

If the block is undefined we will return undefined instead of a boolean. I think we should do !! ( block && block.isValid ) or !! block && block.isValid if we are totally sure isValid is always defined to a boolean.

@youknowriad youknowriad force-pushed the refactor/block-toolbar-perf branch from d264c4e to aa48d43 Compare November 14, 2018 12:27
@youknowriad youknowriad merged commit bb349da into master Nov 14, 2018
@youknowriad youknowriad deleted the refactor/block-toolbar-perf branch November 14, 2018 12:45
@@ -398,6 +398,19 @@ the client ID.

Block name.

### isBlockValid
Copy link
Member

Choose a reason for hiding this comment

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

There was some discussion at #10891 (comment) concerning the naming with regards to placement of adjectives. I think it's particularly relevant here only because we already have precedent with blocks, and with block validation specification, i.e. wp.blocks.isValidBlockContent.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, there seems to be subtle distinction between what's being validated, where the language here might be more appropriate for a known identity, vs. in the block API case of considering some transient inputs.

¯\_(ツ)_/¯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants