-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DataGridPro] Row reorder #4034
[DataGridPro] Row reorder #4034
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
These are the results for the performance tests:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@m4theushw I've updated the demo so if it's ok I think we can merge this and have the feature as part of this week's release. |
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've updated the demo so if it's ok I think we can merge this and have the feature as part of this week's release.
First, do we have enough items to release this week?
Apart from that, the feature is working correctly. 🎉 I only think the docs needs some final touches. We didn't explain how to change the icon and how to set the special __reorder__
field. Most of my suggestions for the core part are simple. We could merge this PR with the docs commented out and open another PR to refine it.
const [loading, setLoading] = React.useState(false); | ||
|
||
React.useEffect(() => { | ||
setRows(data.rows); |
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.
It would be nice to also set __reorder__
. The ID is a hidden field in this dataset.
Here you can check if loading
from useDemoData
is false
, then call setLoading
. Also to prevent the inconsistent state.
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.
It would be nice to also set reorder. The ID is a hidden field in this dataset.
I didn't get that why do we need to set the special column?
For the other part of the feedback - done.
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.
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.
done
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 still see the ID.
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 demo https://deploy-preview-4034--material-ui-x.netlify.app/x/react-data-grid/rows/#row-reorder - yes. I didn't update it but I've added a section explaining how to customize 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.
Personally I found weird because the ID is hidden, but if the first demo should show how the feature works with default options then makes sense to keep as it is.
packages/grid/x-data-grid-pro/src/components/GridRowReorderCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/components/GridRowReorderCell.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/colDef/gridRowReorderColDef.tsx
Outdated
Show resolved
Hide resolved
Question: how i can change the width of reorder column? |
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 think we can add the follow-up issue to the Row ordering doc page to see if it gets upvote traction.
@fourteenmeister I added a section in the docs regarding further customizations https://deploy-preview-4034--material-ui-x.netlify.app/x/react-data-grid/rows/#customizing-the-row-reordering-icon (it will be visible once the build is green) |
Thanks! |
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.
Nice update on docs!
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Happy to see this feature land! A few questions on the UI/UX of the current implementation, we might have opportunities to push it further:
jQuery changed their implement a few months ago https://www.telerik.com/support/whats-new/kendo-jquery-ui: Since we went with the former, I guess to save time as it matches the logic of the column reorders (but with a worse UX, I would advise opening a new issue about it), would it make sense to gray it a bit the row, to help with spatial orientation?
|
Thanks for the feedback @oliviertassinari. For the second question - I agree - there is room for improvement. I did it to match the same behavior we have for the columns. We can create a ticket to update both to match. |
|
I had added some further feedback there this evening, but hadn't seen @oliviertassinari's comments. It seems we share some of the same concerns, but I also raised some additional ones. Copied below, but see linked discussion for context.
|
Thanks for the feedback @oliviertassinari and @mbrookes. I'll organize it into a separate issue today |
Here is the follow up issue #4754 |
Fixes #206
Preview: https://deploy-preview-4034--material-ui-x.netlify.app/x/react-data-grid/rows/#row-reorder
Row reorder
This is a
DataGridPro
feature.The approach I took is the same as the column reorder. I added a new entry in the state to track the dagging row ID. I added one callback -
onRowOrderChange
to match the column reordering in terms of API. The difference between the two callbacks is that I'm not calling the row to reorder one when there is sorting on filtering (thought about it but it has to be called for reach row and that is just too much, but I'm open to suggestions).Last but not least - there is a prop on the grid itself to disable the row reorder as it is enabled by default (same as the column reorder) -
disableRowReorder
.Follow up issue:
TODO:
DataGridPro
row reordering feature on the pricing page material-ui#31875 (to be done after this PR is merged)