-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Automatically select group variation if there is only one available #61871
Automatically select group variation if there is only one available #61871
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
53e517b
to
961a1e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I added an effect into the placeholder component in which I select the variation if there is only one available. I was thinking about adding this logic directly to the Edit component, but it would require calling select for variants for every Edit render so I rather added the change directly into the Placeholder. I'm happy to change it based on the feedback.
Ah yes, this is an interesting question. Looking at the Group block's code, it seems to have been structured to contain the checks that determine if a placeholder should been shown in the useShouldShowPlaceHolder
hook, whereas the Placeholder component doesn't perform any such checks.
Consequently, it's somewhat tempting to preserve this division -- i.e. move the variations check into useShouldShowPlaceHolder
. However, that's unfortunately not the end of the story; we'll also need to add logic to the Edit component itself (probably another logical branch here). The Edit component will then also need to know the block variations, as will the useShouldShowPlaceHolder
hook. I'm not too worried about the performance impact, but it would significantly balloon the code that's needed for this otherwise simple change. It's dubious if shoehorning this new check into the existing structure of the block will really make the code that much more readable if all these changes are needed.
So let's be pragmatic instead and go with your approach 👍
961a1e9
to
777bfe9
Compare
Rebased (to get tests to pass 🤞) |
@ockham Thank you for the review, rebasing and the feedback 🙇
I started with this approach, and after I made a couple of these changes, I decided to go with the simple approach. My thoughts were pretty similar. |
What?
When a core/group block has only one variation available, let's skip the placeholder step and select the variation.
Why?
We are building an email editor, but many email clients don't support modern grids or flex layouts. Therefore, we've disabled most variations, leaving us only with one. It feels weird to display the placeholder step with only one option.
How?
I added an effect into the placeholder component in which I select the variation if there is only one available. I was thinking about adding this logic directly to the Edit component, but it would require calling select for variants for every Edit render so I rather added the change directly into the Placeholder. I'm happy to change it based on the feedback.
Testing Instructions
Default state (multiple variations)
With only one variation
Testing Instructions for Keyboard
The appender in the empty group block is accessible using arrows in the canvas
Screenshots or screencast
Before
After
Closes #62239