Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Display sort column menu items as per sortingOrder prop #7125

Merged
merged 2 commits into from Dec 13, 2022
Merged

[DataGrid] Display sort column menu items as per sortingOrder prop #7125

merged 2 commits into from Dec 13, 2022

Conversation

hanbin9775
Copy link
Contributor

@hanbin9775 hanbin9775 commented Dec 7, 2022 •
edited by cherniavskii

Fixes #7103

@hanbin9775
Copy link
Contributor Author

hanbin9775 commented Dec 7, 2022 •
edited

I couldn't add labels myself. Maybe some permissions are needed? I would be very grateful if somebody could add a label instead of me.

@mui-bot
Copy link

mui-bot commented Dec 7, 2022 •
edited

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 468.2 806.2 655.6 624.82 136.02
Sort 100k rows ms 528.9 1,091.2 748.8 808.98 181.947
Select 100k rows ms 213 257.4 228.3 230.02 15.899
Deselect 100k rows ms 135.9 219.7 199.2 180.4 35.371

Generated by 🚫 dangerJS against c6b858b

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 7, 2022
@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from 3f61b50 to 5d37e52 Compare December 7, 2022 14:14
@MBilalShafi MBilalShafi self-assigned this Dec 8, 2022
@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from 5d37e52 to ab110b1 Compare December 8, 2022 15:40
@hanbin9775
Copy link
Contributor Author

argos keeps failing, because my pr is targeting master branch, not next

@MBilalShafi MBilalShafi added bug 🐛 Something doesn't work feature: Sorting Related to the data grid Sorting feature labels Dec 8, 2022
@MBilalShafi MBilalShafi changed the title [DataGrid] Render SortGridMenuItems according to sortingOrder props [DataGrid] Display sort column menu items as per sortingOrder prop Dec 8, 2022
@MBilalShafi
Copy link
Member

@hanbin9775 Thanks for your contribution. Functionality seems to work fine.
Can we test this new addition please?

@cherniavskii cherniavskii linked an issue Dec 8, 2022 that may be closed by this pull request
[data grid] Remove Unsort from the column menu if sortingOrder is ['asc', 'desc'] #7103
Closed
2 tasks
@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from ab110b1 to c8938f6 Compare December 9, 2022 09:32
@hanbin9775
Copy link
Contributor Author

hanbin9775 commented Dec 9, 2022 •
edited

@MBilalShafi, I didn't fully understood your request but I just add test code for this addition.
If it is could you tell me more about your request? What should I do?

@MBilalShafi
Copy link
Member

I just add test code for this addition.

Thanks, this is exactly what I requested. 👍

@cherniavskii cherniavskii added the needs cherry-pick The PR should be cherry-picked to master after merge label Dec 12, 2022
MBilalShafi
MBilalShafi reviewed Dec 12, 2022
View reviewed changes
packages/grid/x-data-grid-pro/src/tests/columnHeaders.DataGridPro.test.tsx Outdated
@@ -266,5 +266,30 @@ describe('<DataGridPro /> - Column Headers', () => {
expect(menu).not.toHaveFocus();
expect(columnCell).toHaveFocus();
});

it('should display sort column menu items as per sortingOrder prop', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanbin9775 Can you move this test to columnHeaders.DataGrid.test.tsx as the change is done in x-data-grid package?
https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/tests/columnHeaders.DataGrid.test.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. It was my mistake. I thought there were no test codes for x-data-grid package.

@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from c8938f6 to 2bc9b23 Compare December 12, 2022 14:03
MBilalShafi
MBilalShafi reviewed Dec 13, 2022
View reviewed changes
packages/grid/x-data-grid/src/tests/columns.DataGrid.test.tsx Outdated
@@ -15,7 +15,7 @@ const rows: GridRowsProp = [{ id: 1, idBis: 1 }];
const columns: GridColumns = [{ field: 'id' }, { field: 'idBis' }];

describe('<DataGridPro /> - Columns', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously, instead of columns.DataGrid.test.tsx, columnHeaders.DataGrid.test.tsx is a more suitable place for such tests as the menu part belongs to column headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ Sorry again. Fixed right away.

@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from 2bc9b23 to baa3310 Compare December 13, 2022 10:13
@hanbin9775 hanbin9775 force-pushed the render-menu-items-with-sorting-order branch from baa3310 to c6b858b Compare December 13, 2022 10:18
@MBilalShafi MBilalShafi merged commit 56b7cb9 into mui:master Dec 13, 2022
MBilalShafi pushed a commit to MBilalShafi/mui-x that referenced this pull request Dec 13, 2022
@hanbin9775 @MBilalShafi
[DataGrid] Display sort column menu items as per sortingOrder prop (m…
184eb26 
…ui#7125)
@MBilalShafi MBilalShafi mentioned this pull request Dec 13, 2022
Merged
MBilalShafi added a commit that referenced this pull request Dec 16, 2022
@MBilalShafi @hanbin9775
[DataGrid] Display sort column menu items as per sortingOrder prop (#…
cb016ba 
…7125)

Co-authored-by: HanBinKim <37659866+hanbin9775@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MBilalShafi MBilalShafi MBilalShafi approved these changes

@cherniavskii cherniavskii cherniavskii approved these changes

Assignees

@MBilalShafi MBilalShafi

Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Sorting Related to the data grid Sorting feature needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] Remove Unsort from the column menu if sortingOrder is ['asc', 'desc']
5 participants
@hanbin9775 @mui-bot @MBilalShafi @cherniavskii @zannager