ReusableBlockConvertButton: use React hooks. #24015
Conversation
ZebulanStanphill
added
[Type] Code Quality
|
Size Change: +14 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| isReusable || | ||
| // Hide 'Add to Reusable blocks' when reusable blocks are disabled. | ||
| ( canInsertBlockType( 'core/block' ) && | ||
| blocks?.every( |
There was a problem hiding this comment.
This is interesting while every(undefined, something) returns true, undefined?.every( something ) returns undefined (aka false) which changes the meaning.
It's one of the reasons I still prefer lodash defaults over having to think about these subtile usecases. I find something?.something to be very problematic in a lot of usecases and something to use but not to overuse. It can quickly introduce hidden bugs everywhere.
There was a problem hiding this comment.
Lodash's every(undefined, something) returning true is a surprise to me. That's not at all what I would expect.
In this case, though, it seems like it isn't even possible for blocks to be undefined; so the optional chaining shouldn't even be necessary, right?
There was a problem hiding this comment.
Mathematically speaking Every(undefined) = true is the correct thing.
In this case, though, it seems like it isn't even possible for blocks to be undefined; so the optional chaining shouldn't even be necessary, right?
IMO, we should assume things like that. We don't really know at this point what we have in store even if it seems technically impossible. The only way to ensure that is to have typing everywhere.
Maybe:
- When mounting/unmounting blocks, there's a delay where it could happen,
- Maybe there's some async behavior (there isn't) causing this.
What I'm trying to say is that the component should be written consistently regardless of how the rest of the system works.
There was a problem hiding this comment.
In my opinion, running every on undefined isn't something one would assume to be possible by default. That sounds like just as much of a potential gotcha as the optional chaining resulting in false by default.
In this case, I think the current change in behavior is probably fine... if blocks is empty, there's nothing to convert to a reusable block at the moment, so we wouldn't want to show the option, right? In fact, that's what I had assumed the logic was already doing before my changes.
Funny enough, while discussing this, I've just found a bug in the code. We're checking for isReusable || ... when it should be _isReusable || .... Additionally, instead of using optional chaining, we can just use nullish coalescing to default blocks to [] when it is first defined.
I'm going to go implement both of those changes real quick.
ZebulanStanphill
force-pushed
the
update/reusable-block-convert-button-use-hooks
branch
2 times, most recently
from
8e112da to
ff15c9e
Compare
ZebulanStanphill
force-pushed
the
update/reusable-block-convert-button-use-hooks
branch
from
ff15c9e to
5c47241
Compare
|
Alright, I've fixed the |
ZebulanStanphill
added
the
Backport to WP 6.7 Beta/RC
This comment has been minimized.
This comment has been minimized.
|
Whoops, posted that last comment on the wrong PR! Sorry... |
ZebulanStanphill
removed
the
Backport to WP 6.7 Beta/RC
ZebulanStanphill
deleted the
update/reusable-block-convert-button-use-hooks
branch
Description
This PR refactors ReusableBlockConvertButton to use the
useSelectanduseDispatchhooks, rather than the HOC equivalents. It also fixes some comment typos and removes an unnecessary Lodash dependency.Side note: I'm working on a PR to move the "Convert to Regular Block" button to the block toolbar. This PR is intended to polish the code a bit before I make that change.
How has this been tested?
I've tested to make sure the "Add to Reusable blocks" and "Convert to Reusable Block" options appear when they should and work as intended.
Types of changes
Only refactoring + code quality changes. Everything should act the same as
master.Checklist: