Display matching variation icon in Block Switcher #27903
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
the
[Feature] Block Variations
|
Size Change: +66 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
ntsekouras
commented
Dec 28, 2020
gziolo
reviewed
Jan 5, 2021
| @@ -152,4 +148,16 @@ const BlockSwitcher = ( { clientIds } ) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export const BlockSwitcher = ( { clientIds } ) => { | |||
| const blocks = useSelect( | |||
There was a problem hiding this comment.
Why is it necessary? Can't it be consolidated in the existing useSelect hook in the component. Those React hooks limitations aren't great but maybe there is a bit different way.
There was a problem hiding this comment.
In the original implementation, would it be possible to return the client id when there is a single block or the first block's client it when there is multiple blocks of the same type and pass it to useBlockDisplayInformation?
You wouldn't have to expose the icon anymore in such case as it would be calculated from the client id. Unless you want to use a general block type icon when there are multiple blocks selected 😅
There was a problem hiding this comment.
Why is it necessary?
This is just to avoid calling useBlockDisplayInformation if there are no blocks. It could be in one component but I personally don't see it bad... It just bails early.
Unless you want to use a general block type icon when there are multiple blocks selected
I do want this because when we have multiple selected blocks we had two options:
- Show the general block type icon, which seems fine for me as is informative and cheaper to calculate.
- Use the hook for every block to find out if the multiple selected blocks are of the same variation as well. For example if we had two
embedTwitter blocks it should showTwittericon. On the other hand if we had one Twitter and one Youtube, we should fallback to general block type as with the first clientId would show Twitter icon..
There was a problem hiding this comment.
This is just to avoid calling useBlockDisplayInformation if there are no blocks. It could be in one component but I personally don't see it bad... It just bails early.
Doesn't it create another level of components which add a layer of complexity? Is there something else that could be included there so there is no need to pass down all selected blocks?
I do want this because when we have multiple selected blocks we had two options:
Show the general block type icon, which seems fine for me as is informative and cheaper to calculate.
Yes, it makes sense 👍
There was a problem hiding this comment.
Is there something else that could be included there so there is no need to pass down all selected blocks?
Blocks are needed for various things like the transforms preview and the check for being of the same type.
Doesn't it create another level of components which add a layer of complexity?
Just to make sure I understand correctly - Your comment regards on why I have two components BlockSwitcher and BlockSwitcherDropdownMenu, correct?
If that's it, the only reason is just to bail early. I've no strong opinion on that though. I could use a single component, but I'd just have to call the useBlockDisplayInformation always. Do you believe I should do that?
There was a problem hiding this comment.
It's hard to tell because there are multiple conditions to take into account and limitations of React hooks make it only harder. I will test tomorrow with the intent to merge it as is.
|
I can confirm that it works as expected for the block switcher:
Noting that for two blocks with the same block variations it uses the icon from the general block type which is reasonable:
|
gziolo
approved these changes
Jan 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a follow up of #27469, that solved the problem of displaying the proper information of a block, if that block had been created from a block variation.
This PR makes Block Switcher use the proper icon, by trying to find a matching block variation.