DataViews: Support defining field headers/names as React elements. #64642
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] Enhancement
|
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
force-pushed
the
add/header-component
branch
from
5b2a41a to
44f2e57
Compare
7 tasks
|
Size Change: +29 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 44f2e57. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10470876482
|
|
Is it possible to pass the icon to the underlying If not then let's reduce the spacing on the |
|
oandregal
reviewed
Aug 20, 2024
| * The header of the field. Defaults to the label. | ||
| * It allows the usage of a React Element to render the field labels. | ||
| */ | ||
| header?: string | ReactElement; |
There was a problem hiding this comment.
Can we make this a ReactElement and remove the string type? Otherwise, why would you use the header as string if there's already the label field.
Unless you want to enable they being different, which I don't think is the intentionality here.
There was a problem hiding this comment.
It's just because in the NormalizedField type we fallback to the label to avoid having to do the fallback everywhere. But in Field it can indeed be just ReactElement
There was a problem hiding this comment.
To enforce the ReactElement type, when we normalize the field, can't we provide a function that returns the field.label if field.header is not present?
There was a problem hiding this comment.
To enforce the ReactElement type, when we normalize the field, can't we provide a function that returns the field.label if field.header is not present?
Feels more complex to me, especially since string and React element can both be considered "React Element" I wonder if there's a dedicated type for that.
|
The example looks a bit off in the grid layout: 
Not sure this is a blocker for the PR as it's the consumer's responsibility to use the |
oandregal
approved these changes
Aug 20, 2024
youknowriad
force-pushed
the
add/header-component
branch
from
9df5407 to
2de9530
Compare
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
…ordPress#64642) Co-authored-by: youknowriad <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: jameskoster <[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.
Related #55083
What?
While trying to implement dataviews in a third-party project, we had a use-case where we wanted to add an icon and some colors to the table headers.
I think this use-case is common enough that it deserves to be supported in the DataViews component.
How?
In a few places (like filters), we rely on a the label being a string (for instance to render a filter summary), so I think it makes more sense to have two properties here instead of one.
Testing Instructions
I've added an example to the storybook like shown in the image bellow.
Screenshots or screencast