DataViews: Refactor actions to render modal outside the menu #67664
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
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal,
ciampo and
jorgefilipecosta
|
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. |
|
Size Change: +27 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6d82ff8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12195504219
|
ntsekouras
force-pushed
the
dataviews/refactor-actions
branch
from
6d82ff8 to
60a983e
Compare
oandregal
reviewed
Dec 9, 2024
| setActiveModalAction( action ); | ||
| return; | ||
| } | ||
| action.callback( [ item ], { registry } ); |
There was a problem hiding this comment.
I've noticed the addition of the registry. Was some action not working because it didn't have access to it?
There was a problem hiding this comment.
I didn't notice anything in this PR, but it seemed like it wasn't omitted intentionally before (trunk). It's part of the callback props and actions API will be open for extenders in the near future that should have access to registry, no?
There was a problem hiding this comment.
I was curious because if none of the actions actually needed, we could remove it.
oandregal
reviewed
Dec 9, 2024
| <Modal | ||
| title={ action.modalHeader || label } | ||
| __experimentalHideHeader={ !! action.hideModalHeader } | ||
| onRequestClose={ closeModal ?? ( () => {} ) } |
There was a problem hiding this comment.
If we need to keep track of the active action that triggered the modal, is there any case in which the closeModal is not required now?
There was a problem hiding this comment.
(same thought for the RenderModal prop of the ActionModal interface)
There was a problem hiding this comment.
I can't figure out why we had that check in the first place. It seems we always passed that. I'll update the check and the types.
oandregal
reviewed
Dec 9, 2024
oandregal
approved these changes
Dec 9, 2024
bph
changed the title
yogeshbhutkar
pushed a commit
to yogeshbhutkar/gutenberg
that referenced
this pull request
Dec 18, 2024
…Press#67664) Co-authored-by: ntsekouras <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: doekenorg <[email protected]>
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?
Resolves: #66808
Currently on trunk when an action renders a modal, we keep the menu open because the modal is rendered within the
menu item.In many cases some actions don't need to keep the menu open after selecting them and the current implementation leads to more unnecessary clicks for users. Additionally it seems it's a quite common practice to close the menus after selecting an action (see comment) .
This PR closes the menu when a user selects an action which also matches the behavior for actions that do not render a modal.
Testing Instructions
actions and bulk actionsin DataViews should work as before. You should also explicitly checklistlayouts, as they were using the actions dropdown a bit differently, due to different markup for keyboard navigation.