Control sorting through props for iTwin and iModel tables - #223
Conversation
| * Controlled sort model. When provided, the table's sort state is fully | ||
| * controlled by the parent and must be kept in sync via `onSortModelChange`. | ||
| */ | ||
| sortModel?: GridSortModel; |
There was a problem hiding this comment.
It looks like this just effects the table view, not the tile/card view. My initial thought is that would be good to keep sorting unified between both views. WDYT?
There was a problem hiding this comment.
i updated the changes to apply to both views as you suggested
|
|
||
| /** Supported IModel sorting types */ | ||
| export type IModelSortOptionsKeys = "name" | "createdDateTime"; | ||
| export type IModelSortOptionsKeys = "name" | "lastChangesetPushDateTime"; |
There was a problem hiding this comment.
createdDateTime still seems valid - any reason to remove it?
There was a problem hiding this comment.
included back createdDateTime. I initially removed it since the table doesnt contain a create date column. But I kept it so you can sort it from an external sorter
| {...args} | ||
| orderbyOptions={orderbyOptions} | ||
| onSortModelChange={(newSortModel: ITwinTableSortModel) => { | ||
| action("sort model changed")(newSortModel); |
There was a problem hiding this comment.
for some reason I'm not seeing the action output - are you? I wonder if this callback is working
There was a problem hiding this comment.
it works when clicking on the table headers to sort the table
| {...args} | ||
| sortOptions={sortModel} | ||
| onSortModelChange={(newSortModel: IModelTableSortModel) => { | ||
| if (newSortModel.length > 0) { |
There was a problem hiding this comment.
nit: this should have an action to match iTwin Grid - I also didn't see anything firing when I tried it
There was a problem hiding this comment.
added the action as suggested, the action fires when you click on the table column headers. The idea is to be able to sync the table sorting with external sorting used in the app.
| onSortModelChange={(newSortModel: IModelTableSortModel) => { | ||
| if (newSortModel.length > 0) { | ||
| const newSort = newSortModel[0]; | ||
| setSortModel({ |
There was a problem hiding this comment.
Is there any reason to call setSortModel here when it's already being called in the click handlers? Same for iTwinGrid.
There was a problem hiding this comment.
as described in my other comment, the click handlers (chips at the top of the table) are not part of the iModelGridMUI, theyre just there in the story to simulate sorting done outside of the iModelGrid (similar to the sorting dropdown available in studio). So an app using iModelGrid should sync its sorting logic when the sort is changed using the table header sorting.
There was a problem hiding this comment.
I wonder if we can unify/simplify the naming a bit.
setSortModel({sortType: newSort.field, descending: newSort.sort === "desc"});
setOrderbyOptions(`${newSort.field} ${newSort.sort ?? "asc"}`);
sortOptions={{"sortType": "name", "descending": false}}
onSortModelChange
I find the name "model" a bit confusing, although I see it comes from MUI Datagrid (I think?)
As it stands, it seems we supply sort in the orderByOptions="name asc" style, but the callback receives an array of {field, sort} objects?
This seems like a chance to tidy up our naming and make things consistent, if possible.
There was a problem hiding this comment.
updated the name to onOrderbyOptionsChange to match current orderbyOptions prop in iTwinGrid and it also receives the same format (string "column asc/desc")
There was a problem hiding this comment.
The rename definitely helps. WDYT about moving to a proper tuple like {field, direction} for these MUI components? While we're the only consumers of MUI it feels like a rare opportunity to make API changes. The existing behaviour of passing these $odata strings feels brittle to me.
I might even be tempted to do [{field, direction}] (array) since the API allows sorting like displayName asc, createdDateTime asc
There was a problem hiding this comment.
ya passing string as props is definitely not ideal, its lacking typing/structure. I do think going the array route as you suggested is the way to go. Its also compatible with the dataGrid sorting in MUI
Co-authored-by: Alex Dunae <alex@dunae.ca>
…nents with updated view mode and action logging
…ents with new sort options handling
| // Translate the `sortOptions` prop into the equivalent DataGrid sort model so | ||
| // the table view reflects the requested sort without reordering the fetched | ||
| // list (which keeps its default sort). | ||
| const initialTableSortModel = React.useMemo<IModelTableSortModel>( |
There was a problem hiding this comment.
WDYT about making this internal to IModelTableMUI - my thinking is we keep the same "public" props for as long as possible for simplicity and then just convert them at the last minute for DataGrid
| sort.sortType === "name" | ||
| ? iModel.displayName ?? iModel.name ?? "" | ||
| : iModel[sort.sortType] ?? ""; | ||
| : iModel.lastChangesetPushDateTime ?? iModel.createdDateTime ?? ""; |
There was a problem hiding this comment.
I don't think this will work as intended.
if sortType === 'createdDateTime' we'll use iModel.lastChangesetPushDateTime if it has value
Probably worth writing a little test file for this helper since it's nice and isolated
There was a problem hiding this comment.
you're right this change was made before I added back the createdDateTime. The issue was some iModels had lastChangesetPushDateTime set as null/undefined and it was putting that iModel at the top of the sorting list, will fix it and add a test as suggested
There was a problem hiding this comment.
The rename definitely helps. WDYT about moving to a proper tuple like {field, direction} for these MUI components? While we're the only consumers of MUI it feels like a rare opportunity to make API changes. The existing behaviour of passing these $odata strings feels brittle to me.
I might even be tempted to do [{field, direction}] (array) since the API allows sorting like displayName asc, createdDateTime asc
| export type IModelSortOptionsKeys = | ||
| | "name" | ||
| | "createdDateTime" | ||
| | "lastChangesetPushDateTime"; |
There was a problem hiding this comment.
On further thought, this lastChangesetPushDateTime doesn't apply to non-MUI table view (I think). All the more reason to have a separate IModelSortOptionsMUI type
| onSortModelChange={(model) => | ||
| onSortModelChange?.([...model] as IModelTableSortModel) | ||
| } | ||
| sortingOrder={sortModel ? ["asc", "desc"] : ["asc", "desc", null]} |
There was a problem hiding this comment.
Any reason for this conditional? I think these could always be sortingOrder={["asc", "desc"]} since we don't really allow an undefined sort order. We're always having some sort of sort (lol).
There was a problem hiding this comment.
The default sorting order of mui is ["asc", "desc", null]
https://mui.com/x/api/data-grid/data-grid/#data-grid-prop-sortingOrder
this allows user to not have a sort on the table

Problem is with a controlled sort you always have to have a sorting set which is why we set it to ["asc", "desc"].
There was a problem hiding this comment.
Could we get rid of the "null" sort option in MUI?
Or, does it make sense to have a "no sort" option for both grids entirely? IIRC even if the consumer doesn't provide a sort order, we still default to name asc. Not sure what that would really gain though...
Added the ability to control the sorting of the tables through props.
