-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Move insertionPoint state to block-editor store/rename existing insertionPoint to insertionCue #65098
Conversation
Size Change: +607 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
I was also trying to find the right solution for this issue and came to a similar conclusion. We should probably fully revert #64048 and work on an alternative fix for #63866. Why revert? Zoom-out mode is still experimental (behind the settings flag); experimental features should hinder stable ones. |
@Mamaduka - I'm fine reverting #64048 as well. I've been looking at this most of the day and feel like there's a deeper issue at play. The insertion point is bound to the editor, and gets set when the inserter opens. The idea of the inserter staying open and the insertion point changing wasn’t a thing when this was designed, because the inserter was supposed to close and reset when focus was moved out of it. Now that it stays open and you can interact with the canvas, we have this muddling of states where the insertion point can change while the inserter is open. |
I worked more on this today to move the insertion point into the block editor package. It's working correctly for the zoom out inserters. I really dislike the naming of all the things as it's so hard to untangle which insertion point means what :/ |
cc @WordPress/gutenberg-core |
* @param {string} value.filterValue A query to filter the inserter results. | ||
* @param {Function} value.onSelect A callback when an item is selected. | ||
* @param {string} value.tab The tab to open in the inserter. | ||
* @param {string} value.category The category to initialize in the inserter. |
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.
These are not new, just adding the documentation for things already in use.
- _value.filterValue_ `string`: A query to filter the inserter results. | ||
- _value.onSelect_ `Function`: A callback when an item is selected. | ||
- _value.tab_ `string`: The tab to open in the inserter. | ||
- _value.category_ `string`: The category to initialize in the inserter. |
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.
These are not new, just adding the documentation for things already in use.
} else if ( | ||
insertionPoint?.insertionIndex && | ||
insertionPoint?.rootClientId | ||
) { | ||
_destinationRootClientId = insertionPoint.rootClientId; | ||
_destinationIndex = insertionPoint.insertionIndex; |
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.
Putting this after the insertionIndex
or clientId
to preserve the existing API. This lets anyone who passes these values in via the props to still have full control.
export function setInserterInsertionPoint( value ) { | ||
return { | ||
type: 'SET_INSERTER_INSERTION_POINT', | ||
value, | ||
}; | ||
} | ||
|
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 don't like this naming, but it's at least clear 🤷🏻 I'm concerned that this assumes only one instance of the inserter, while the editor package way passes it in. That said, it's still possible to manually pass the inserter the root block and index and it will be respected.
case 'SELECT_BLOCK': | ||
return null; |
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.
Clear the insertion point after a new block selection.
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.
In my testing, the vertical displacement is still visible after selecting a different block.
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. |
392038a
to
a90bb7f
Compare
Flaky tests detected in 98cf472. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11055136565
|
Performance charts after the revert PR was merged. We should monitor "Inserter Hovering Items" results while working on the fix. Screenshot |
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 noticed something weird that I can't reproduce on trunk. I'm not sure if it's related to this PR nor what's going on. The testing environment is Playground.
Kapture.2024-09-11.at.16.29.35.mp4
Notice how sometimes selecting the pattern will insert it at the wrong place. And some other times hover over sections will rerender the patterns inserter.
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.
Thanks for taking this one. Some things I noticed.
_destinationRootClientId = insertionPoint?.rootClientId | ||
? insertionPoint.rootClientId | ||
: rootClientId; | ||
_destinationIndex = insertionPoint.insertionIndex; |
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.
The other conditions have comments to explain them. Could we add one here?
setInserterIsOpened( { | ||
rootClientId, | ||
insertionIndex, | ||
filterValue, | ||
onSelect, | ||
} ); | ||
setInserterInsertionPoint( { rootClientId, insertionIndex } ); |
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.
We could have the editor stores setInserterIsOpened
dispatch the setInserterInsertionPoint
to the block editor store as part of the action. That said it might be clearer this way as the "set inserter is opened" communicates "toggling" rather than "setting an insertion point".
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 think we might want to setInserterInsertionPoint
without setInserterIsOpened
as well - for example if you're using the quick inserter.
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.
But what about backwards compat on the setInserterIsOpened
API?
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.
What I mean is that I think its good to have the flexibility of calling these independently rather than tying one to the other.
packages/editor/src/store/actions.js
Outdated
* @param {string} value.rootClientId The root client ID to insert at. | ||
* @param {number} value.insertionIndex The index to insert at. |
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.
We could continue to make this backwards compatible by dispatching the new action to the block editor store if the rootClientId
or the insertionIndex
are provided as part of value
. You could also fire a deprecated notice in the console.
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 like this idea.
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 API has changed. It's a public API. We have to ensure it's backwards compatible.
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.
You can still pass these things, they just don't do anything within our implementation of the block editor. I'll add a deprecation and fire that though.
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.
...they just don't do anything
That was the bit I was concerned about because if a public API used to do something it needs to continue to do that.
Looks like you fixed it up now 👍
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.
We can’t deprecate a public API/feature and suggest to use a private API instead.
It would be non-actionable for consumers.
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 think it would also be fine to not deprecate any of it. You can still pass it those props, it just won't do anything in our codebase 🤷🏻
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.
Yeh actually we don't need to deprecate it do we? @Mamaduka is right that we can't point to a private API - great spot there.
I have a couple of surface notes:
Screenshot |
- old block-editor insertionPoint becomes insertionCue (for the reducer and state value) - which opens up insertionPoint to be the real insertion point - so we can change getInserterInsertionPoint to getInsertionPoint - rename editor private getInsertionPoint to getInserter to more accurately describe its state - remove unnecessary deprecation
a0f66e9
to
24a2aec
Compare
describe( 'setInsertionPoint', () => { | ||
it( 'should return the SET_INSERTION_POINT action', () => { | ||
expect( | ||
setInsertionPoint( { | ||
rootClientId: '', | ||
index: '123', | ||
} ) | ||
).toEqual( { | ||
type: 'SET_INSERTION_POINT', | ||
value: { rootClientId: '', index: '123' }, | ||
} ); | ||
} ); | ||
} ); |
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 test and all the other tests in this file aren't doing anything useful.
The important thing for actions is that reducers do things based on them and the selectors read out a specific value. The parts of a store shouldn't be tested in isolation like this.
It looks like you're trying to fix the breaking change that may have happened some time earlier, but updating the test and documentation isn't the way to fix it. The prior logic needs to be restored.
If the tests were for the store as a whole, maybe this would have been caught.
Also, to be clear, I'm suggesting moving the tests, documentation, and fixing the setIsInserterOpened
regression described in your comment into a different PR.
packages/editor/src/store/actions.js
Outdated
return registry.dispatch( blockEditorStore ).setInsertionPoint( { | ||
rootClientId: value.rootClientId, | ||
index: value.insertionIndex, | ||
} ); |
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.
There are two bugs here:
- It shouldn't return here, it should just dispatch it.
setInsertionPoint
isn't an available action because it's in the private-actions in the block editor store. Are we allowed to use it in this editor package if it's private in the block editor package?
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.
Looks good to me. There's more to do in #65290, but this is a good start.
@jeryj, I appreciate all the work done here, but I think getting broader feedback from the core team would have been nice. |
@Mamaduka - I'd be love to have gotten more feedback on this as well. It had been open long enough that I didn't think I was going to hear anything else, and it was also blocking follow-up work on insertion point work with the zoom out view. Very happy to iterate or follow-up with any of it! Fortunately, I ended up finding a way to not change any public APIs, so everything is possible to iterate or undo. |
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function setInsertionPoint( value ) { |
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.
How is this different than showInsertionPoint
?
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.
Ok, so If I'm understand properly we have:
showInsertionPoint
which is the exact location where the block will be inserted. So the true insertion point.- and we have
setInsertionPoint
which is about the top level "inserter" component. IT's about setting the "default" insertion point (if blocks are not allowed to be inserted there, we compute the true insertion point and show it using showInsertionPoint)
is that correct?
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.
Clarifying my understanding more:
The "inserter insertion point" state was being set in the "editor" package because the "Inserter" component has a "rootClientId" and "clientId" props, so this one is controlled externally, I guess the idea is that at the same time, you can potentially render different inserters with different default insertion points. The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.
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.
How is this different than showInsertionPoint?
showInsertionPoint
makes the insertion cue visible. It is not code-wise related to the insertion point, just a visual indicator.
showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.
No, this only shows where the blue line will get shown (what I will now refer to as the insertion cue for clarity). We don't have a true insertion point at the moment other than passing an insertion point to useInsertionPoint
(what the sidebar inserter was doing) or via the new setInsertionPoint
added in this PR.
The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.
Exactly. No props were removed and a manually passed insertion point to the inserter will still be respected. We needed to move it to the block-editor state to clear the insertion point when a block is selected.
This reverted PR demos more about why we need this. Code-wise, the inserter was built with a static insertion point in mind. Now the inserter can be open with selection on the block canvas happening, meaning the insertion point can be updated while the inserter is still open.
The problem with the PR above is that it relied on getBlockInsertionPoint
which updates constantly on hover, so we had to revert it.
I wrote an overview of the Insertion API current state here: #65290
* @param {Object} state | ||
* @return {Object} where the insertion point in the block editor is or null if none is set. | ||
*/ | ||
export function getInsertionPoint( state ) { |
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.
How is this different than getBlockInsertionPoint
?
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.
getBlockInsertionPoint
is really about the blue insertion cue that gets shown. getBlockInsertionPoint
is constantly updated based on where you're hovering the canvas. Maybe originally getBlockInsertionPoint
was about where the insertion would happen, but its code now is tied to where the insertion cue shows.
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.
What's the difference for you between "insertion cue" and "insertion position". If we're showing a different insertion cue than the actual insertion point, then we have problem.
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.
The difference is that one is an index and the other one is an UI element?
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.
Both are state for me. The UI element is just a reflexion of a state.
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 think it gets tricky when you include drag and drop. You’re showing an insertion cue at a location that may be different than what is set via selection.
If you update that point on hover via drag and drop, this is the correct behavior for the insertion cue/insertion point, but a lot of the things built off using it were not intended to be updating their insertion point that often.
So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.
One thing I'm not sure I understand is how is this blocking things in zoom-out? |
We need to be able to update the insertion point with the inserter panel still open. This isn't zoom-out specific, as it's useful for the main canvas as well. We also have work on the zoom out separators that is behaving in a buggy way, and being able to get a clear insertion point will be very helpful. |
Can you expand more, I'm not sure how the current change is related to that. Because the previous |
You could change the insertion point via the old There very well may be better way to handle this, and I'm very open to that. The way I went with here seemed like the best first step towards clarifying and increasing flexibility of the insertion methods. |
What?
In the block editor store:
insertionPoint
reducer toinsertionCue
(as it is where the blue line shows up not where insertion is guaranteed to happen).insertionPoint
to be used to manually pass an insertion point (such as when clicking an inline inserter button like the blue zoom out inserter buttons)setInsertionPoint
actiongetInsertionPoint
selectorIn the editor store:
setIsInserterOpened
dispatchessetInsertionPoint
to the block editor storegetInsertionPoint
togetInserter
to more clearly communicate this is the state of the inserter, not solely about the insertion point.Why?/How
Now that the inserter can remain open while interacting with the block canvas, it is expected that when you select a new block, the insertion will occur after the selected block.
The insertion point is handled by the editor package via
setIsInserterOpened( value )
, which can take an object to open the inserter to a certain tab panel and also provide the place where the insertion should happen. We want to change this insertion point when selecting a new block on the canvas. Block selection is handled via the block editor package. Since the block editor should not know about the editor, and we can't rely on theSELECTION_CHANGE
action to clear the insertion point within the editor store, I've moved the insertion point data to be within the block editor package.In order to clear up confusion, I've also renamed some state and selectors:
insertionPoint
state becomesinsertionCue
, as this more accurately names it. It was not the insertion point, but the place where we want to show the blue line on the canvas.insertionPoint
to be used for manually setting an insertion point, if needed, such as when opening the inserter via one of the inline inserter buttons.getInsertionPoint
is renamed togetInserter
, as this was a point of confusion. It is only the insertion point if the inserter is open and becomes stale data as soon as another interaction on the canvas happens. This is about the sate of the inserter when it is opened, and if it should still be open.getInserter
is a better name, and it could still be better.getInserterPanel
maybe?How?
state.insertionPoint
tostate.insertionCue
in the block editor store and related action/reducers.state.insertionPoint
to store when an insertion point is manually passed.setInsertionPoint
action in the block editor store whensetIsInserterOpen
has an root index and insertion point set.insertionPoint
state onSELECT_BLOCK
getInsertionPoint
togetInserter
to more accurately communicate that this is the state the inserter initialized to rather than the final insertion point.Testing Instructions
For pattern insertion points in Zoom Out
Default Editing
Testing Instructions for Keyboard
Screenshots or screencast