Data views: Add ability to display fields as a badge in grid layout #60284
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
jameskoster
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. |
|
Size Change: +375 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1a4c96a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8468074865
|
ntsekouras
reviewed
Apr 1, 2024
| @@ -68,6 +68,8 @@ const defaultConfigPerViewType = { | |||
| [ LAYOUT_GRID ]: { | |||
| mediaField: 'preview', | |||
| primaryField: 'title', | |||
| displayAsBadgeFields: [ 'sync-status' ], | |||
There was a problem hiding this comment.
I think we should declare a badge fields once and has nothing to do with asColumnFields. Let me refactor the code a bit..
There was a problem hiding this comment.
In terms of naming, I think we should remove the displayAs prefix everywhere. We already use badgeFields in the UI component below, and there's no difference from the existing primaryField and mediaField (all of this is about "marking" a set of fields and display them differently).
The same applies to columnFields, that can be changed in a different PR.
|
PR tests well for me, good improvements, good to move forward. A few notes:
Contrast here looks on the low side.
Before we introduce a separator here, can we stress test whitespace as a separator a bit more? Semantically it doesn't feel like a great place to separate rather than space out. |
oandregal
reviewed
Apr 1, 2024
oandregal
reviewed
Apr 1, 2024
| @@ -111,6 +111,11 @@ | |||
| .dataviews-pagination { | |||
| z-index: z-index(".edit-site-patterns__dataviews-list-pagination"); | |||
| } | |||
|
|
|||
| .dataviews-view-grid__field-sync-status.is-badge:has(.edit-site-patterns__field-sync-status-fully) { | |||
There was a problem hiding this comment.
I'd prefer if we don't target dataviews classes in consumer code, as it implies it's a public API. Can we instead add a specific class in the sync-status field? We have access to the view type there.
I've noticed that we do this for pagination, pattern actions, and templates/parts actions. cc @ntsekouras
There was a problem hiding this comment.
Personally I don't think it's bad to use a specific class with a field's id suffix and I wouldn't worry right now about indicating public API, since we can have breaking changes in the DataViews package.
Having said that, I agree with you that we don't need this right now. I updated the code.
|
Thanks for the updates y'all. @jasmussen I removed the separator: 
Note: The above is just an example. Page fields still display with their labels: 
Status can potentially be a badge, but should be visually aligned with the designs shared in #60289 (comment) which makes it follow-up territory. |
ntsekouras
approved these changes
Apr 4, 2024
oandregal
reviewed
Apr 4, 2024
| @@ -111,6 +111,11 @@ | |||
| .dataviews-pagination { | |||
| z-index: z-index(".edit-site-patterns__dataviews-list-pagination"); | |||
| } | |||
|
|
|||
| .is-badge:has(.edit-site-patterns__field-sync-status-fully) { | |||
There was a problem hiding this comment.
Wouldn't this work?
Suggested change
| .is-badge:has(.edit-site-patterns__field-sync-status-fully) { | |
| .edit-site-patterns__field-sync-status-fully { |
There was a problem hiding this comment.
That suggestion would produce this:
The styles need to be applied to the element that contains .edit-site-patterns__field-sync-status-fully.
We can probably do that without the is-badge class though. I'll try something.
oandregal
reviewed
Apr 4, 2024
packages/dataviews/src/view-grid.js
Outdated
| key={ field.id } | ||
| className={ classnames( | ||
| 'dataviews-view-grid__field-value', | ||
| 'is-badge' |
There was a problem hiding this comment.
It sounds like we may remove the is-badge class? It's only used in packages/edit-site/src/components/page-patterns/style.scss but it seems to be unnecessary?
oandregal
approved these changes
Apr 4, 2024
youknowriad
reviewed
Apr 5, 2024
| @@ -73,6 +73,7 @@ const defaultConfigPerViewType = { | |||
| [ LAYOUT_GRID ]: { | |||
| mediaField: 'preview', | |||
| primaryField: 'title', | |||
| badgeFields: [ 'sync-status' ], | |||
There was a problem hiding this comment.
We're starting to have several kind of displays for fields: primary, media, badge (maybe others in other layouts). It would be good to check if we can consolidate somehow. Maybe visibleFields could support arrays as fields: [ [ { field: 'preview', display: 'media' }], { field: 'title', display: 'primary' }], { field: 'sync-status', display: 'badge' }, 'anyrandomfield' ]
There was a problem hiding this comment.
I also wonder if want to show the "badge" in the same way in other views. Meaning maybe the field itself (sync status) display as a badge in its render function rather than this begin a config.
There was a problem hiding this comment.
Yes I think so too. I'll create an issue to track this.
This was referenced Apr 5, 2024
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
…ordPress#60284) Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
29 tasks
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?
This PR does a few things which can be boiled down to three main changes:
Why?
Follow-up to #60083, which introduced the ability to arrange fields in a 'column' layout and forced the display of field labels.
This change is for fields where the label is not necessary, IE it can be interpreted from the value alone.
Of the current field set across all data views, 'sync status' in patterns is an appropriate consumer: There is no need for the verbose: "Sync status: Synced" when "Synced" alone will do.
As data views expand to other areas (e.g. Media Library: #55238) this affordance will become more useful. Media is a good example because media files have many fields that do not require a label (size, file name, type, dimensions, etc.). This PR will make it possible to implement the following mockup:
How?
displayAsBadgeFieldsparameter toGridItem, similar todisplayAsColumnFieldsHStackabove all other fields.field.idas a classname on all field outputs.defaultConfigPerViewTypein the Patterns page to specific thatsync-statusis a badge field.sync-statusfield render to include class name based on valueTwo other minor tweaks:
Testing Instructions
Before
After
For more advanced testing you can temporarily swap fields in the Pages data view to display as badges by opening
packages/edit-site/src/components/sidebar-dataviews/default-views.jsand updatingDEFAULT_CONFIG_PER_VIEW_TYPElike so:Then:
badges.mp4
Note: It may be desirable to display Status as a badge with different styling for different values, similar to sync status. We can explore that in a follow up.