[DataGrid] Row spanning #14124
[DataGrid] Row spanning #14124
Conversation
MBilalShafi
added
the
component: data grid
…en there are repeated values
There was a problem hiding this comment.
I am wondering what the behavior should be with editing.
Should editing a cell that spans multiple rows update all the repeating values? It makes sense to me.
Although is not how colSpan behaves (example: https://stackblitz.com/edit/react-ucq5za?file=Demo.tsx).
What do you think @mui/xgrid ?
There was a problem hiding this comment.
I don't understand user expectations very well for this feature, but I was wondering if there is a case where users would want to change the value of individual cells that are part of a row span?
Should editing a cell that spans multiple rows update all the repeating values?
If not, this suggestion makes sense to me.
There was a problem hiding this comment.
I see use cases for both
- If you have structured data and you use rowspan to show value of the parent once for all rows, you would probably want to update all of them at once
- If we connected cells in a column for an age, then you probably don't want to update all of them at once
There was a problem hiding this comment.
If we connected cells in a column for an age, then you probably don't want to update all of them at once
I believe such rows shouldn't be spanned in the first place. I introduced a prop rowSpanValueGetter to allow excluding unwanted values from being spanned.
For example with age, as you mentioned, it could be the same yet belong to different person. So if we avoid spanning them, we could go with the assumption: editing a cell that spans multiple rows would update all the repeating values
Consider this example: https://deploy-preview-14124--material-ui-x.netlify.app/x/react-data-grid/row-spanning/#customizing-row-spanned-cells
George Floyd and Cynthia Duke are both 25 years old but rowSpanValueGetter is used to correctly span the values.
Does it make sense?
There was a problem hiding this comment.
for our pretty automated feature, this gives enough initial control to cover both cases
There was a problem hiding this comment.
After thinking more about this, I'm inclined to not go with this approach for now, mainly because it is a breaking behavioral change.
Currently, on every cell or row update, processRowUpdate prop gets fired once.
If we go with the suggested behavior, the processRowUpdate will get fired more than once each edit (depending on the impacted rows).
The current behavior might also be useful in some cases where the users intend to modify the spanned cells based on the values.
break-spanned-cells.mp4
And the column spanning works in a similar way too, so it should be fine for now.
We might wait for some user feedback around this before deciding to go with the suggested approach. And even if we decide to go with it, we do it at a major, to avoid breaking changes.
github-actions
bot
added
the
PR: out-of-date
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
PR: out-of-date
github-actions
bot
added
the
PR: out-of-date
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
PR: out-of-date
In my initial testing, the basic cell selection worked fine, did you spot any specific interaction not properly working? |
It should be released this week or coming week. |
Did not test it. I meant more to validate that we want it to behave the way it is (even if it means it can't be used together) |
Co-authored-by: Sycamore <[email protected]> Signed-off-by: Bilal Shafi <[email protected]>
| return ( | ||
| <div | ||
| data-colindex={colIndex} | ||
| role="presentation" |
There was a problem hiding this comment.
This shouldn't be used, no?
| role="presentation" |
Looking https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/presentation_role, I see no purpose for the role here.
There was a problem hiding this comment.
Actually, this seems needed. If I understand https://www.w3.org/WAI/ARIA/apg/practices/hiding-semantics/ correctly, because the parent is a role="row", it needs each child to have a cell role, since this one doesn't have it, it should be hidden.
Signed-off-by: Bilal Shafi <[email protected]> Co-authored-by: Sycamore <[email protected]>
|
Hello, can you merge this feature to the v5 version of MUI? |
|
Hey @tomekxoxo, We only backport critical bug fixes to the previous major versions, I'd suggest you to consider upgrading to a new major. We have the migration guides available for every major release upgrade. Some of the changes could even be automatically applied using codemods. Here are the migration guides:
I don't think the row spanning is integrated with Excel export as of now. |
🎉 Feature Completed 🎉
Resolves #207
Based on https://www.notion.so/mui-org/xGrid-Row-Spanning-b178fd43907146799eb7a57d9330b68b?pvs=4#160e28c1b4b247d7befe0cbf44acd475
Documentation and Preview: https://deploy-preview-14124--material-ui-x.netlify.app/x/react-data-grid/row-spanning/
Todos
colDef.valueGettercolDef.colSpanValueGetterfor custom use-casescolSpando?Follow-up Todos
rowIndex. [DataGrid] Row spanning #14124 (comment)Changelog
💫 Support Row spanning on the Data Grid which automatically merges the consecutive cells in a column