DataViews: Type the BulkActionsToolbar component #61673
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
youknowriad
added
[Type] Code Quality
|
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. |
youknowriad
commented
May 15, 2024
| } } | ||
| onActionPerformed={ () => { | ||
| setActionInProgress( null ); | ||
| } } |
There was a problem hiding this comment.
I found that these were useless.
There was a problem hiding this comment.
Hi @youknowriad I think we need these. When an action is being executed in a modal we still to call setActionInProgress( action.id ); when an action begins and when the action ends setActionInProgress( null );. If we don't do this in an action like delete where the available actions and the number of items change the toolbar will be changing during the action execution.
There was a problem hiding this comment.
but these props don't seem present on the component?
There was a problem hiding this comment.
The props are present on the render modal:
There was a problem hiding this comment.
Sure but then the props need to be added to ActionWithModal if we really want this callback here. I removed the passing of the props, not the props themselves, I didn't touch the component personally.
There was a problem hiding this comment.
Is there any bug that I can test with to understand that?
youknowriad
commented
May 15, 2024
| onClick={ () => { | ||
| setActionInProgress( action.id ); | ||
| action.callback( selectedItems, () => { | ||
| setActionInProgress( action.id ); | ||
| } ); |
There was a problem hiding this comment.
I think callbacks don't have a second argument, so that code was probably useless too.
youknowriad
commented
May 15, 2024
| const selectedEligibleItems = useMemo( () => { | ||
| return selectedItems.filter( ( item ) => { | ||
| return action.isEligible( item ); | ||
| return ! action.isEligible || action.isEligible( item ); |
There was a problem hiding this comment.
This was not handling the case where actions don't have isEligible defined.
|
Size Change: -16 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Other than a comment I left here #61673 (comment) the PR looks good 👍
jorgefilipecosta
approved these changes
May 15, 2024
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.
Related #55083
Follow-up to #61185
What?
The DataViews package is a types heavy package. There's a lot of structures that need to be documented properly. So this continues the effort to add explicit types to the package. This PR focuses on typing the BulkActionsToolbar component.
Testing instructions
There's no functional change, it's essentially a code quality + documentation change.