Close inspector on pattern category select #60004
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
|
Size Change: +123 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
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. |
draganescu
added
[Type] Enhancement
draganescu
changed the title
aaronrobertshaw
approved these changes
Mar 20, 2024
There was a problem hiding this comment.
This is working well for me @draganescu 👍
Screen.Recording.2024-03-20.at.11.36.26.AM.mp4
Initially I thought I'd have to add it to the post editor but then I saw we auto close the inspector the moment the inserter is open there. This should be unified too I think.
Agreed.
The e2es failures are unrelated and due to docker startup failure so I've just kicked them off again.
Mamaduka
reviewed
Mar 20, 2024
I think that only happens on smaller screens. I would love to see user feedback on this change. Personally, I would be annoyed that I have to re-open the sidebar every time I browse patterns. |
I can confirm this. Turns out I had dev tools open to the right while testing earlier. |
Mamaduka
reviewed
Mar 20, 2024
| @@ -65,6 +68,9 @@ export default function InserterSidebar() { | |||
| insertionPoint.insertionIndex | |||
| } | |||
| __experimentalFilterValue={ insertionPoint.filterValue } | |||
| __experimentalOnPatternCategorySelection={ | |||
| isRightSidebarOpen ? closeGeneralSidebar() : undefined | |||
There was a problem hiding this comment.
Suggested change
| isRightSidebarOpen ? closeGeneralSidebar() : undefined | |
| isRightSidebarOpen ? closeGeneralSidebar : undefined |
We should pass the function.
richtabor
reviewed
Mar 20, 2024
|
@richtabor I think this has value w/o zoom out as well since inserting patterns is a more "splashy" effect on the canvas and more space to see it is better. |
Let's give it a try like this. |
|
Closely related to #59775, as that effort enables zoom out when viewing patterns. |
Yea, I feel that too. Why I was thinking about engaging this behavior in zoom-out mode. The difference here is that selecting a pattern category engages a third sidebar area on the canvas, that will remain open per #59741 as you add patterns to the page. This feels a bit much:
|
|
I agree it's a bit much and it's made visible by the drag and drop e2e test failure in my PR. One idea could be to hide the inserter temporarily while dragging. When the inserter is open, the main value of the "canvas" is when you're dragging, otherwise it's not that important. |
Downside there is that the canvas will shift once you initiate a drag, which I don't think will be ideal. |
|
I will update the behaviour to only occur in zoom out mode. |
draganescu
force-pushed
the
add/close-inspector-on-pattern-list-open
branch
from
f4f65a9
to
a7c4d19
Compare
ntsekouras
reviewed
Mar 27, 2024
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* close inspector on pattern category select * send stable references to the pattern category click handler * fix passing the callback not calling it, add behavior to post editor * only toggle if zoom out mode experiment is on
youknowriad
reviewed
May 29, 2024
| @@ -45,6 +46,7 @@ function InserterMenu( | |||
| showMostUsedBlocks, | |||
| __experimentalFilterValue = '', | |||
| shouldFocusBlock = true, | |||
| __experimentalOnPatternCategorySelection = NOOP, | |||
There was a problem hiding this comment.
This seems like a new public API. Just a reminder to stop adding prefixed experimental APIs like that. It's preferred to add private APIs going forward. Thanks.
There was a problem hiding this comment.
Yes, good point! 🤦🏻 I copied #56773 verbatim. Would a linter that says no when we use __experimental help ... probably not 😁
There was a problem hiding this comment.
There are is still time to make it private for 6.6, no?
There was a problem hiding this comment.
Opened #62110 but I am a bit unclear how to proceed with privatizing a component that was public but marked experimental - since that will create a public experimental and a private experimental component :/
There was a problem hiding this comment.
Alternative PR here: #62130. Would love some testing to land this for 6.6.
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.
What?
Close #59963
Why?
How?
Mostly copy #56773
Testing Instructions
Initially I thought I'd have to add it to the post editor but then I saw we auto close the inspector the moment the inserter is open there. This should be unified too I think.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
hide-inspector.mp4