-
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
Inserter: Fix 'Browse All' in Quick Inserter #26443
Conversation
} | ||
} else { | ||
// Otherwise, we're in "auto mode" where the insertion point is | ||
// decided by 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.
Do you think we can simplify further and make this the default mode: Maybe by "setting the insertion point" when we open the inserter (all inserters)
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 a really tough problem to solve.
Ideally the insertion point would be deterministic, derived from other state, but realistically there's probably no way to do that reliably with the way Browse all
opens a completely unrelated component.
So this seems like a logical step, and especially so given there's already state used for showing the blue line.
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Outdated
Show resolved
Hide resolved
setTimeout( () => { | ||
setInserterIsOpened( true ); | ||
} ); | ||
setInsertionPoint( rootClientId, blockIndex ); |
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.
For consistency it feels to me like setInsertionPoint
should be set when an appender button is clicked rather than 'Browse All', otherwise the sidebar block library becomes a special case that uses this state, while the quick inserter doesn't.
Having said that, I haven't looked too deeply into how the quick inserter works, so could be wrong on this.
case 'REPLACE_INNER_BLOCKS': | ||
case 'INSERT_BLOCKS': | ||
case 'REMOVE_BLOCKS': | ||
case 'REPLACE_BLOCKS': |
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.
It would be easy to miss adding an action here that should clear this state 😬
The potential issue with holding the insertion point as state will be the potential for the block list to become out of sync with the insertion point state.
I found a similar issue here with block selection (note to self- finalize this PR) - #21598
Should RESET_BLOCKS
be here too?
destRootClientId = getBlockRootClientId( end ); | ||
let _destinationRootClientId, _destinationIndex; | ||
|
||
if ( rootClientId || insertionIndex || clientId || isAppender ) { |
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 was a relevant comment here about these props - #25763 (comment).
So the idea would to refactor inserters to just use rootClientId and blockIndex. The insertion point state seems to fit well with that, which is nice (no action required here 😄 ).
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 that idea. It's definitely a little crazy right 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.
Tested and it seems to work well 👍
); | ||
blockIndex: getBlockIndex( clientId, rootClientId ), | ||
}; | ||
}, [] ); |
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.
Should this have clientId, rootClientId
as dependencies?
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.
Fixed in 0ac181d
// Otherwise, we're in "auto mode" where the insertion point is | ||
// decided by getBlockInsertionPoint(). | ||
|
||
_destinationRootClientId = 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.
We can call getBlockInsertionPoint
only once here and get its results then.
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.
Fixed in e707a30
What's stopping us from rendering the sidebar inserter in the same react tree as the quick inserter, and creating a sidebar slot to render it there for the DOM? A similar thing is doen for the quick inserter. It as also rendered in a Popover slot, but part of the block react tree. |
You're always thinking with portals, @ellatrix! 🙂 It's an interesting idea. So in effect, I'll play with it a little. I don't see why it couldn't work but if working on this bug has taught me anything it's that there are a lot more edge cases than you'd think. |
I tried a Slot/Fill approach and ran into a problem regarding closing the inserter sidebar. Say that a user clicks on a block appender and then selects Browse All. Now, say that the user closes the left sidebar by e.g. opening the right sidebar, clicking the + button, or clicking the X button (only appears on mobile). Or, alternatively, say that the user decides to not insert a block and instead move their cursor to the bottom of the post. In both cases, we need to unmount the |
It would fill only if the appender is focussed right? |
No, because focus moves to the left sidebar's search field. If we unmount the |
Could something like the click outside or focus outside HoC work? I guess the behaviour is much like a popover or modal, just positioned differently. |
Hey: |
Hi, would any of the reviewers of this PR be interested in helping move it forward? It seems we got stuck between the current (working) solution and a (still hypothetical) completely different one. If no one is able to help, what are the thoughts on merging more or less as-is and refactoring later if needed? |
I agree with @tellthemachines that we should favor solving the issue first before further refactorings of how the inserter is rendered. |
I'm happy to have a go at addressing some of the feedback. Probably worth making the selectors/actions unstable if there's the potential for removing them, so I'll do that to. |
Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'.
f6e1ed1
to
a473f2e
Compare
Size Change: -814 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
a473f2e
to
4b7fdeb
Compare
4b7fdeb
to
90a1820
Compare
* | ||
* @return {Object} Insertion point object with `rootClientId`, `index`. | ||
* @return {Object} Insertion point object with `rootClientId` and `index`. |
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.
Previously in this PR getBlockInsertionPoint
also returned isVisible
, but I've removed that as it seems to be unused.
I need to look into the e2e test failures, seems like I broke something 😄 edit: ah yep, the |
I'm running into a weird bug while testing this branch:
Seems like the insertion point is being set inside the Columns block instead of the Column. Other than that, testing in both post and site editors shows this working much better than on master. |
@tellthemachines Yeah, I can reproduce, slightly different results, the available blocks was initially correct, but then changed to just column when my mouse left the block library. |
Ah yep, the 938c7a5 fixes. |
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! Everything is working well now ✅
🕺 |
Thanks for picking this up @talldan! |
* Inserter: Fix 'Browse All' in Quick Inserter Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'. * Inserter: Fix typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Call getBlockInsertionPoint once * Add useSelect dependencies * Make setInsertionPoint unstable * Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered * Make index required and clarify rootClientId usage * Split insertionPoint into two reducers * Fix getInsertionPoint selector that expects default state of reducer to be null * Avoid resetting insertionPoint state on HIDE_INSERTION_POINT Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
* Cover block: Restore default overlay background (#26569) * Restore default Cover overlay background The default Cover block overlay background was removed. This changed the style of existing blocks on existing sites. Restore the default background in such a way that it does not conflict with theme-provided background-color styles and there is no need to artificially add extra specificity. Fix regression: #26545 * Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552) * Cover Block: Restore opacity controls (#26625) * Fix image block centering when resizing to a smaller size (#26376) * Fix image block centering when resizing to a smaller size * Revert previous 100% width fix * Remove display: table when image block is resized to avoid centering of block * Match frontend classes for alignment in editor * Gallery: Remove caption fallback for alt attribute (#26676) * Fix quote block default alignment (#26680) * Gallery: Remove obsolete deprecation entry (#26736) * Do not apply text color if it is being overriden (#24626) * Fix: Preset colors don't work on button block style outline (#26707) * Tests: Add fixture for Column deprecation (#26774) * Fix/column width units (#26757) * Fix issues with different units in column widths * Columns with fixed width should keep width on recalculation * Address review feedback * fix undefined index notice in Social Link Block (#25663) * fix undefined index notice * use isset instead of array_key_exists check * Update packages/block-library/src/social-link/index.php Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> * Adds in missing styling for toolbar when using text-only setting (#26769) * Adds in missing styling for toolbar when using text-only setting * Increases margin * Moves style to correct file * Inserter: Fix 'Browse All' in Quick Inserter (#26443) * Inserter: Fix 'Browse All' in Quick Inserter Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'. * Inserter: Fix typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Call getBlockInsertionPoint once * Add useSelect dependencies * Make setInsertionPoint unstable * Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered * Make index required and clarify rootClientId usage * Split insertionPoint into two reducers * Fix getInsertionPoint selector that expects default state of reducer to be null * Avoid resetting insertionPoint state on HIDE_INSERTION_POINT Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Jon Surrell <jon.surrell@automattic.com> Co-authored-by: Jacopo Tomasone <Copons@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Bernie Reiter <ockham@raz.or.at> Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: Oliver Juhas <webmandesign@users.noreply.github.com> Co-authored-by: Jorge Costa <jorge.costa@automattic.com> Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de> Co-authored-by: Tammie Lister <tammie@automattic.com> Co-authored-by: Robert Anderson <robert@noisysocks.com>
Fixes #24403.
Related to #24285.
Maintain the insertion point in
@wordpress/block-editor
state when moving from the Quick Inserter to the Inserter.This fixes a bug (#24403) where the insertion point is wrong after clicking a block appender (e.g. in a Columns block) and selecting Browse All.
Care is taken to not regress a previous bug (#24262) where the insertion point was wrong after clicking an inline insertion button and selecting Browse All.
I re-used the existing
insertionPoint
state tree so as to minimise the amount of new actions and selectors needed. This means we have three insertion point actions:setInsertionPoint
: Sets where the inserter will default to inserting blocks.showInsertionPoint
: Sets where the inserter will default to inserting blocks and display this to the user.hideInsertionPoint
: Hides the insertion point from the user.The insertion point is cleared when block selection changes. This is needed for the Site Editor where the Inserter is persistent.
Testing
A regression E2E test has been added. This complements the one added in #24396.
The manual flows that I checked are:
And: