Conversation
|
Size Change: -423 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
Thanks for taking on this challenge. This is one of the most complicated blocks there is. I haven't tested the branch yet, but it looks like some of the end-to-end test failures may be legit. |
af85cce to
2d03090
Compare
| noticeOperations, | ||
| onReplace, | ||
| } ) { | ||
| const selected = useSelect( ( select ) => { |
There was a problem hiding this comment.
You could use destructuring right here rather than destructuring selected immediately after.
There was a problem hiding this comment.
This has been discussed numerous times and I'm not sure if we reached a conclusion on what to do in case of clashing variable names. Cc @youknowriad
There was a problem hiding this comment.
My way of doing it is to rename the variables inside the useSelector callback because that's the "temporary" part and I see the returned values as a bit more important to get semantically right.
I also like inline useSelect callbacks because the separate selector function is not something that work consistently and you lose some context.
There was a problem hiding this comment.
I prefix the temporary variables with an underscore, e.g. _thing; that's the approach I'm using in #21427.
|
Because as far as I remember, there wasn't full browser support. If that's not the case anymore, we can maybe make a lint rule? |
|
I'm going to leave the |
|
@ellatrix We're polyfilling JS functions (including But yeah, I can just change it in one of my PRs. And also, a lint rule would definitely be useful. |
Description
This PR rewrites the image block with hooks. I'm doing this so we can also rewrite the
ImageSizecomponent to a hook which would allow us to simplify the markup.Best viewed without whitespace changes: https://github.com/WordPress/gutenberg/pull/22499/files?diff=split&w=1
How has this been tested?
Screenshots
Types of changes
Checklist: