-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: Add groupByField support to grid layout #70752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
5339afb
2ceb8b8
117f4c0
aeac8fc
fef3502
703aa6c
5b8663f
1a733ad
d97929e
e2235dc
8ac0e2c
4aef87a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ import { | |
| FlexItem, | ||
| privateApis as componentsPrivateApis, | ||
| } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { useInstanceId } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
|
|
@@ -293,51 +293,142 @@ function ViewGrid< Item >( { | |
| gridTemplateColumns: `repeat(${ usedPreviewSize }, minmax(0, 1fr))`, | ||
| } | ||
| : {}; | ||
|
|
||
| const groupField = view.groupByField | ||
| ? fields.find( ( f ) => f.id === view.groupByField ) | ||
| : null; | ||
|
|
||
| // Group data by groupByField if specified | ||
| const dataByGroup = groupField | ||
| ? data.reduce( ( groups: { [ key: string ]: typeof data }, item ) => { | ||
| const groupName = groupField.getValue( { item } ); | ||
| if ( ! groups[ groupName ] ) { | ||
| groups[ groupName ] = []; | ||
|
||
| } | ||
| groups[ groupName ].push( item ); | ||
| return groups; | ||
| }, {} ) | ||
| : null; | ||
|
|
||
| return ( | ||
| <> | ||
| { hasData && ( | ||
| <Grid | ||
| gap={ 8 } | ||
| columns={ 2 } | ||
| alignment="top" | ||
| className={ clsx( 'dataviews-view-grid', className ) } | ||
| style={ gridStyle } | ||
| aria-busy={ isLoading } | ||
| > | ||
| { data.map( ( item ) => { | ||
| return ( | ||
| <GridItem | ||
| key={ getItemId( item ) } | ||
| view={ view } | ||
| selection={ selection } | ||
| onChangeSelection={ onChangeSelection } | ||
| onClickItem={ onClickItem } | ||
| isItemClickable={ isItemClickable } | ||
| renderItemLink={ renderItemLink } | ||
| getItemId={ getItemId } | ||
| item={ item } | ||
| actions={ actions } | ||
| mediaField={ mediaField } | ||
| titleField={ titleField } | ||
| descriptionField={ descriptionField } | ||
| regularFields={ regularFields } | ||
| badgeFields={ badgeFields } | ||
| hasBulkActions={ hasBulkActions } | ||
| /> | ||
| ); | ||
| } ) } | ||
| </Grid> | ||
| ) } | ||
| { ! hasData && ( | ||
| <div | ||
| className={ clsx( { | ||
| 'dataviews-loading': isLoading, | ||
| 'dataviews-no-results': ! isLoading, | ||
| } ) } | ||
| > | ||
| <p>{ isLoading ? <Spinner /> : __( 'No results' ) }</p> | ||
| </div> | ||
| ) } | ||
| { | ||
| // Render multiple groups. | ||
| hasData && groupField && dataByGroup && ( | ||
| <VStack spacing={ 4 }> | ||
| { Object.entries( dataByGroup ).map( | ||
| ( [ groupName, groupItems ] ) => ( | ||
| <VStack key={ groupName } spacing={ 2 }> | ||
| <h3 className="dataviews-view-grid__group-header"> | ||
| { sprintf( | ||
| // translators: 1: The label of the field e.g. "Date". 2: The value of the field, e.g.: "May 2022". | ||
| __( '%1$s: %2$s' ), | ||
| groupField.label, | ||
| groupName | ||
| ) } | ||
| </h3> | ||
| <Grid | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like this just duplicates what we have in the bottom of the component, only diff seems to be the data used no?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it is. I didn't think this would benefit from a further abstraction.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for something big really here to be honest, just a function or subcomponent. |
||
| gap={ 8 } | ||
| columns={ 2 } | ||
| alignment="top" | ||
| className={ clsx( | ||
| 'dataviews-view-grid', | ||
| className | ||
| ) } | ||
| style={ gridStyle } | ||
| aria-busy={ isLoading } | ||
| > | ||
| { groupItems.map( ( item ) => { | ||
| return ( | ||
| <GridItem | ||
| key={ getItemId( item ) } | ||
| view={ view } | ||
| selection={ selection } | ||
| onChangeSelection={ | ||
| onChangeSelection | ||
| } | ||
| onClickItem={ onClickItem } | ||
| isItemClickable={ | ||
| isItemClickable | ||
| } | ||
| renderItemLink={ | ||
| renderItemLink | ||
| } | ||
| getItemId={ getItemId } | ||
| item={ item } | ||
| actions={ actions } | ||
| mediaField={ mediaField } | ||
| titleField={ titleField } | ||
| descriptionField={ | ||
| descriptionField | ||
| } | ||
| regularFields={ | ||
| regularFields | ||
| } | ||
| badgeFields={ badgeFields } | ||
| hasBulkActions={ | ||
| hasBulkActions | ||
| } | ||
| /> | ||
| ); | ||
| } ) } | ||
| </Grid> | ||
| </VStack> | ||
| ) | ||
| ) } | ||
| </VStack> | ||
| ) | ||
| } | ||
|
|
||
| { | ||
| // Render a single grid with all data. | ||
| hasData && ! dataByGroup && ( | ||
| <Grid | ||
| gap={ 8 } | ||
| columns={ 2 } | ||
| alignment="top" | ||
| className={ clsx( 'dataviews-view-grid', className ) } | ||
| style={ gridStyle } | ||
| aria-busy={ isLoading } | ||
| > | ||
| { data.map( ( item ) => { | ||
| return ( | ||
| <GridItem | ||
| key={ getItemId( item ) } | ||
| view={ view } | ||
| selection={ selection } | ||
| onChangeSelection={ onChangeSelection } | ||
| onClickItem={ onClickItem } | ||
| isItemClickable={ isItemClickable } | ||
| renderItemLink={ renderItemLink } | ||
| getItemId={ getItemId } | ||
| item={ item } | ||
| actions={ actions } | ||
| mediaField={ mediaField } | ||
| titleField={ titleField } | ||
| descriptionField={ descriptionField } | ||
| regularFields={ regularFields } | ||
| badgeFields={ badgeFields } | ||
| hasBulkActions={ hasBulkActions } | ||
| /> | ||
| ); | ||
| } ) } | ||
| </Grid> | ||
| ) | ||
| } | ||
| { | ||
| // Render empty state. | ||
| ! hasData && ( | ||
| <div | ||
| className={ clsx( { | ||
| 'dataviews-loading': isLoading, | ||
| 'dataviews-no-results': ! isLoading, | ||
| } ) } | ||
| > | ||
| <p>{ isLoading ? <Spinner /> : __( 'No results' ) }</p> | ||
| </div> | ||
| ) | ||
| } | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -375,16 +375,32 @@ export function filterSortAndPaginate< Item >( | |
| } | ||
|
|
||
| // Handle sorting. | ||
| if ( view.sort ) { | ||
| const fieldId = view.sort.field; | ||
| const fieldToSort = _fields.find( ( field ) => { | ||
| return field.id === fieldId; | ||
| } ); | ||
| if ( fieldToSort ) { | ||
| filteredData.sort( ( a, b ) => { | ||
| return fieldToSort.sort( a, b, view.sort?.direction ?? 'desc' ); | ||
| if ( view.sort || view.groupByField ) { | ||
| filteredData.sort( ( a, b ) => { | ||
| // Sort by the group by field. | ||
| const groupByField = _fields.find( ( field ) => { | ||
| return field.id === view.groupByField; | ||
| } ); | ||
| } | ||
| if ( view.groupByField && groupByField ) { | ||
| const groupCompare = groupByField.sort( a, b, 'asc' ); | ||
|
|
||
| // If items are in different groups, return the group comparison result. | ||
| // Otherwise, fall back to sorting by the sort field. | ||
| if ( groupCompare !== 0 ) { | ||
| return groupCompare; | ||
| } | ||
| } | ||
|
|
||
| // Sort by the sort field. | ||
| const sortByField = _fields.find( ( field ) => { | ||
| return field.id === view?.sort?.field; | ||
|
Comment on lines
+381
to
+396
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change moved the computation of This should be: sortByField = view.sort && find(…)
groupByField = view.groupByField && find(…)
if (sortByField || groupByField) {
filteredData.sort((a, b) => {
if (groupByField) { … }
if (sortByField) { … }
return 0
} )
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed this at #70822 Theoretically, this could have a performance hit in situations where the field to sort/group by wasn't found. In practice, that field is defined by the consumer, or it's the result of users clicking around — unless there's a bug, the provided field will always exist. Is that your assessment as well?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so, but I didn't think much of that scenario. I was drawn more to the unnecessary recomputing and sorting. :) |
||
| } ); | ||
| if ( view.sort && sortByField ) { | ||
| return sortByField.sort( a, b, view.sort?.direction ?? 'desc' ); | ||
| } | ||
|
|
||
| return 0; | ||
| } ); | ||
| } | ||
|
|
||
| // Handle pagination. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -419,6 +419,11 @@ interface ViewBase { | |
| * Whether to show the hierarchical levels. | ||
| */ | ||
| showLevels?: boolean; | ||
|
|
||
| /** | ||
| * The field to group by. | ||
| */ | ||
| groupByField?: string; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any thoughts on a UI to configure this? (not necessary for this PR) Also would be good to document this in the README.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done at d97929e
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The obvious way is to add a new select in the view config somewhere. Alternatively, we could explore making it part of the field list (similarly to what we have for the media field). |
||
| } | ||
|
|
||
| export interface ColumnStyle { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if just the
datachanges? Don't we risk showing obsolete memoized data that way?I know this is in a story, but folks tend to copy code from stories, so we need to be mindful about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case,
dataandfieldswon't change because they are static arrays.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I was clear enough - my worry is about people copying the code from stories and it potentially leading to the issues I mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that viewpoint, but I'm reluctant to add unnecessary code to cover misuse — that's not something I've seen us doing.