Open convert to links modal on select of a page item #48723
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
draganescu
changed the title
draganescu
requested review from
MaggieCabrera,
ajlende and
scruffian
and removed request for
ajitbohra and
tellthemachines
draganescu
added
[Type] Enhancement
|
Size Change: +136 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
scruffian
reviewed
Mar 3, 2023
scruffian
reviewed
Mar 3, 2023
scruffian
reviewed
Mar 3, 2023
draganescu
force-pushed
the
add/auto-edit-page-links
branch
from
c6786e3 to
af57fe2
Compare
draganescu
requested review from
ellatrix,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
draganescu
force-pushed
the
add/auto-edit-page-links
branch
from
af57fe2 to
e9bcf96
Compare
|
Is there any way that we can pass the information needed for |
|
I wonder if we can have an |
|
@ajlende not really. I tried before this approach using the onChange prop of the innerblocks and simulate a fake change via setAttributes see f596d2e but it's clunky to say the least. Because we don't render the page list items individually I can't pass a handler for some of the stuff they do. However I think the approach here is quite OK. What is wrong with relying on the dragged state of an inner block? |
I don't like that we have to rely on a It feels too much like a hack, I guess. |
|
I don't feel it is a hack to have a block act on how its children change in UI state. I feel more like the animation API is lacking a good enough implementation so that it can be canceled. I think the To that effect - no pun intended - maybe so that we're both happy half way I should just copy the modal and the conversion function into the page list item too (for now) and move the logic to the page list item block out of the page list block. That way, we wouldn't care about parents and children, and would render the modal in the page list item block. I'll try this in a new commit to see how complicated it would be. The modal would still not be self contained because we need to render it when either one of isSelected or isDragged are true, not following some event, but following a state change. |
|
This is feeling pretty good to me. I'll refer to developers on the precise implementation.
Separate issue: could/should the navigation block retain focus when clicking edit? |
Absolutely - I even tried to make it happen, doesn't work yet :D EDIT: Now I see what you mean @jasmussen - after the conversion. That is a problem in |
draganescu
requested review from
tellthemachines
and removed request for
ellatrix,
ryanwelcher,
juanmaguitar and
fabiankaegy
Agreed. |
|
Looking good @draganescu! Should the modal appear on item drop rather then drag? Shouldn't block releasing this but might be a follow up refinement if we think it makes sense. |
ajlende
approved these changes
Mar 7, 2023
There was a problem hiding this comment.
I still find it wrong that we have these hasSelectedInnerBlock and hasDraggedInnerBlock functions, but it may be a limitation of the system we have for passing data down to inner blocks.
Approving because there is good value being added in this change, and doing it in what I would consider to be the "right" way would likely involve modifying the system for inner blocks.
| }; | ||
| }, | ||
| [ clientId ] | ||
| ); | ||
|
|
||
| const innerBlocksProps = useInnerBlocksProps( blockProps, { |
There was a problem hiding this comment.
@draganescu and I looked into passing the convertToNavigationLinks function down into the child block where the modal could be opened from there, but we couldn't find where the props ended up since they didn't get added to the edit props.
There was a problem hiding this comment.
Another option could have been creating a context just for this scenario that the data needed in the child could have access to, but that also seems a little convoluted.
There was a problem hiding this comment.
Do you think it's worth looking into modifying the system for inner blocks?
ajlende
added
the
Backport to WP 6.7 Beta/RC
This was referenced Mar 8, 2023
|
I'm tentatively removing backport label for this PR. While it's an excellent enhancement, I don't think we have enough time to properly test and follow up with any fixes if required during this release cycle. |
Mamaduka
removed
the
Backport to WP 6.7 Beta/RC
Mamaduka
reviewed
Mar 8, 2023
| * | ||
| * @return {boolean} Whether the block has an inner block dragged | ||
| */ | ||
| export function hasDraggedInnerBlock( state, clientId, deep = false ) { |
There was a problem hiding this comment.
We can also do this inline in the useSelect callback and avoid introducing a new selector for just one use case.
Here's a similar example:
gutenberg/packages/edit-site/src/components/save-button/index.js
Lines 23 to 25 in d19dbd6
There was a problem hiding this comment.
I want this new selector, it's as useful as hasSelectedInnerBlock and this is a perfect opportunity to introduce it via using it.
There was a problem hiding this comment.
Just sharing my personal preference of avoiding adding selector to packages scope, when I can :)
This was referenced Mar 13, 2023
This was referenced Jul 11, 2023
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?
When users interact with the Page List Items in the Navigation List View, we should invoke the "convert to links" modal, to make it clear that these items can't be edited.
Questions
cc @SaxonF @richtabor
Fixes #47064
Why?
To make it clear that these items can't be edited, and allow users to do so.
How?
By using the onChange prop of
innerBlocksin the page list template and simulating a change viasetAttributtes. A cleaner way may exist.Testing Instructions
Testing Instructions for Keyboard
As above.
Screenshots or screencast
convert-on-interaction.mp4