DataViews: label prop in Actions API can be either a string or a function
#61942
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
|
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. |
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
youknowriad,
sirreal,
jorgefilipecosta and
mcsf
and removed request for
youknowriad
|
Size Change: +117 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4274b02. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9269058081
|
youknowriad
reviewed
May 24, 2024
| @@ -470,7 +470,15 @@ const viewPostAction = { | |||
|
|
|||
| const postRevisionsAction = { | |||
| id: 'view-post-revisions', | |||
| label: __( 'View revisions' ), | |||
| getLabel( items ) { | |||
There was a problem hiding this comment.
What if instead of a new property getLabel, we just make the label string | ReactComponent
There was a problem hiding this comment.
A React component would allow calling selectors, hooks ... in additional to using the "items".
There was a problem hiding this comment.
I didn't want to mix a prop with such different types, but don't have a strong opinion on this. A component could do that, but we use the label in specific internal components that render the actions and they expect a string returned value.
Do you think it would be enough to add docs that we support a label component, but expect a string value?
There was a problem hiding this comment.
they expect a string returned value.
What are these places?
There was a problem hiding this comment.
I've just looked into the dataviews package, and I think we can support components in all the places where action.label is used now. That said, I would not mind having other opinions here. For block types, we have __experimentalLabel which is basically always a component that is kind of equivalent.
Any thoughts @mcsf @ellatrix @jorgefilipecosta
There was a problem hiding this comment.
they expect a string returned value.
Sorry, I should rephrase.. What I meant is that the label is used in places (menu items, tooltips, modal header title, etc..) that expect a final 'string' value - not per JS type per se. So while we can support components technically, personally I'd rather not do it from the start to try to keep consistency of the UI that is handled by the internal packages and not third party consumers.
__experimentalLabel is equivalent yes, and also returns a string value in our codebase. If we returned something different - that we could - it would definitely create problems in UI. if you see most of the implementations of this callback it's either passed the block attributes, which is like our selected items or have a hook to get the current entity.
So, while I don't have a strong opinion to use a component, I'd rather we are just as flexible as we need right now. I don't think we have a use case right now for needing components, do we?
| items[ 0 ]._links?.[ 'version-history' ]?.[ 0 ]?.count ?? 0; | ||
| return sprintf( | ||
| /* translators: %s: number of revisions */ | ||
| __( 'View revisions (%s)' ), |
There was a problem hiding this comment.
This approach works but I'm not sure from the design point of view if the idea was to use a suffix in the button, something like:
It would make things complex in terms of API, as we would need something besides the label.
There was a problem hiding this comment.
Things work well on this PR, and the code looks good. I'm ok with a getLabel function for now that just returns a string if needed in the future we can always switch to a component to allow hooks usage etc.
My only doubt is if the number of revisions should be shown as part of the label or as suffix but that's a design question, from the code point of view things look good.
The label is not used only in TBH, having a component or getter is a small detail (although I prefer a simple function). What I'm not sure after @youknowriad 's comment is whether we should keep two props for getting the label or having one that could be a string or a function. I'm still leaning towards two props for clarity, but no strong opinion. |
jorgefilipecosta
approved these changes
May 28, 2024
|
I don't like the two properties for the same thing but it's fine by me. |
ntsekouras
force-pushed
the
add/getlabel-dataviews-actions
branch
from
4c3b603
to
871c894
Compare
ntsekouras
commented
May 28, 2024
packages/dataviews/README.md
Outdated
| @@ -241,7 +244,7 @@ Collection of operations that can be performed upon each record. | |||
| Each action is an object with the following properties: | |||
|
|
|||
| - `id`: string, required. Unique identifier of the action. For example, `move-to-trash`. | |||
| - `label`: string, required. User facing description of the action. For example, `Move to Trash`. | |||
| - `label`: string|function, required. User facing description of the action. For example, `Move to Trash`. In case we want to adjust the label based on the selected items, a getter function which accepts the selected records as input can be provided. The getter function should always return a `string` value. | |||
There was a problem hiding this comment.
Besides this change, everything else was from auto formatting.
ntsekouras
changed the title
getLabel in Actions APIlabel prop in Actions API can be either a sting or a function
ntsekouras
changed the title
label prop in Actions API can be either a sting or a functionlabel prop in Actions API can be either a string or a function
ntsekouras
force-pushed
the
add/getlabel-dataviews-actions
branch
from
74f0f4a
to
4274b02
Compare
|
Apologies for not catching this, but do you think it would be feasible to use the |
|
|
Would it be massively over-complicating to include a separate prop for the tooltip content, so that you could specify:
|
I think it will add complexity at the API and TBH I'm not sure it will add much value in general for every action. Do we see many more actions to have something similar? If we update the actions rendering in the future, would that API make sense? So, I think while we could explore this and try to evaluate an API for splitting labels, suffix etc.., I don't think it's something that could be done for 6.6, given the timeline. |
bph
added
Needs User Documentation
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
…`function` (WordPress#61942) Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…`function` (WordPress#61942) Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
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: #61867
This PR allows
labelprop in Actions API can be either astringor afunction, in case we want to use information from the selected items. The function accepts the selected items, in case an action wants to modify the label based on that information.It makes more sense for non bulk actions and in this PR is used for the post revisions action to also display the number of available revisions.
Since the action's label is used mostly in places that expect a string, I think it makes sense not to have a component, but just a callback that accepts the selected items and returns a
string.Testing Instructions
Ensure that actions and their labels work properly:
The only visible change from this PR is that post revisions action label includes the number of the revisions.