-
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
Improve parent block selector button UI #23800
Conversation
Size Change: -318 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
f70b126
to
d167f30
Compare
I was planning to add a gap between the parent-selector and the rest of the toolbar: But adding that extra space is a lot harder than it may seem. The outermost borders and background color are provided by the Perhaps it is okay to not have the gap? It's worth noting that something like a thicker border would be a lot easier to do, albeit a bit non-standard. Then again, separating the parent-selector with a gap even though it's technically still part of the toolbar is also non-standard. So what do you think I should do? Personally, I'm okay with the current design in this PR. |
This is currently what I'm seeing right now: The parent icon does not have a right border. Looking at your mockups though, I still prefer the detached parent icon. The disconnection of the two help identify it as something tangentially related to the selected block, and not as part of the selected block itself. |
Well, I'm gonna need help to get it looking like the mockups. |
d167f30
to
7f02e92
Compare
Also, I rebased and tested the branch myself and there's no missing border for me. I've tested on the Twenty Twenty theme, though I don't think the theme should make any difference. Could you try again now that the branch has been rebased? |
I dig this, though I think having the space between the parent button and the block toolbar is needed — otherwise its not totally obvious what block is currently selected; Its strange to have two block icons in (visually) single toolbar. I think you could try adding some margin to the toolbar, and then |
7f02e92
to
b318b49
Compare
Good idea, @shaunandrews! I did just that, and here's what it looks like now: The CSS is a bit more complicated now, but it's still better than what's in |
* @param {Function} [props.onChange=noop] Callback function. | ||
*/ | ||
export function useDebouncedShowMovers( { | ||
export function useDebouncedActivateCallbacks( { |
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.
This is now only being used for the block outline shown when the block icon is hovered. I could follow up with another PR to remove the debouncing entirely, if desired.
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.
I have tested this and the code LGTM 🚢
Thanks for exploring this. I have to say the proposal gives me a lot of pause as it turns something that was convenient but not primary into the most prominent aspect of the block toolbar, visible at all times. Considering nested blocks are going to become more and more the norm, I think we need to be more careful here:
And we get out of bounds much more easily: Given this has the potential of significantly changing the experience by introducing a permanently visible item, I think we should wait a bit more and continue to explore possible solutions. |
The out-of-bounds thing isn't supposed to happen. I think that issue happens on The other points are definitely valid, though. Feel free to post suggestions in #23766.
The iteration without the gap between the parent selector and the rest of the toolbar doesn't have this problem, but it still has all the other issues mentioned, unfortunately. |
b318b49
to
ba3866b
Compare
I've removed the gap between the parent-selector-button and the rest of the toolbar. We ought to be avoiding special-case styling like that. Here's what it looks like after rebasing the PR to include all the latest UI changes in The inconsistent usage of block icons to mean two different things is still a problem. I'm going to post some ideas about how to move forward in #23766, but for now I wanted to go ahead and push an update to this PR to prevent it from getting too stale. |
265c149
to
012fda8
Compare
Following my latest idea in #23766, I've once again changed how the "Select parent" option is presented. This time, I've moved it out of the main part of the block toolbar and into the "More options" menu. I believe this approach solves every problem with prior designs except, perhaps, the issue of discoverability. Please let me know what you think about this design in #23766. |
012fda8
to
c414501
Compare
69af0b9
to
80f7032
Compare
80f7032
to
e324a60
Compare
I'm looping back on this one. Thanks for all the iterations and tweaks so far, much appreciated! I'm coming around to the fact perhaps we need slightly different treatments for some of these contexts. For example, the initial design proposed here matches well for this use case: #28522. Likely also for Gallery. The case of social icons and buttons is a bit tricky since the container is really there to facilitate creation of multiple items, so for that one I feel we need to work on the Unsure if we should be applying the same treatment to parents that are indifferent to its children (Cover, Group, Columns, Template Parts). So my current thinking is to split the work in three:
|
In terms of inner blocks that would leave us with three configurations:
And also include the “select parent” in the ellipsis menu from this PR for mobile and redundancy. |
@ZebulanStanphill sorry missed this comment. I think in mobile the situation is still not great and the ellipsis button could be useful, but it's definitely less pressing than before with all the latest tools (parent selector, list view, etc). Another place from where I think it should be possible to select a parent container is the sidebar inspector, particularly on blocks that are semantically tied (buttons:button, navigation:navigation-link, social-icons:social-icon). |
I forgot this PR was still open. Closing, since #40105 superseded it. |
Description
Closes #23766.
Checklist: