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.

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

Fix DataGrid sorting for initialInput 'DESC' #1596

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

rdonigian
Copy link
Contributor

@rdonigian rdonigian commented Oct 3, 2024

@rdonigian rdonigian requested a review from CarsonF as a code owner October 3, 2024 14:08
@rdonigian rdonigian force-pushed the bugfix/dashboard-sorting branch from 7231b70 to 778678a Compare October 3, 2024 14:13
@CarsonF CarsonF force-pushed the bugfix/dashboard-sorting branch from 778678a to 030759f Compare October 3, 2024 15:10
@CarsonF
Copy link
Member

CarsonF commented Oct 3, 2024

I was bothered by the null chaining here. We know we have a sort item, so I updated types to reflect that.

I also thought that your logic was a bit hard to follow. And it is by chance that the sort selection order is asc -> desc -> null. I think this could be flipped around separately. So I think what I have is more clear on what we are actually doing and ensure we flip the order correctly regardless of grid configuration.

What do you think?

@rdonigian
Copy link
Contributor Author

I was bothered by the null chaining here. We know we have a sort item, so I updated types to reflect that.

I also thought that your logic was a bit hard to follow. And it is by chance that the sort selection order is asc -> desc -> null. I think this could be flipped around separately. So I think what I have is more clear on what we are actually doing and ensure we flip the order correctly regardless of grid configuration.

What do you think?

@rdonigian rdonigian closed this Oct 3, 2024
@rdonigian rdonigian reopened this Oct 3, 2024
@rdonigian
Copy link
Contributor Author

I like that approach, I was fighting the optional chaining. LGTM

@CarsonF CarsonF force-pushed the bugfix/dashboard-sorting branch from 030759f to e3f1153 Compare October 3, 2024 16:04
@CarsonF CarsonF enabled auto-merge October 3, 2024 16:04
@CarsonF CarsonF merged commit cf61276 into develop Oct 3, 2024
3 checks passed
@CarsonF CarsonF deleted the bugfix/dashboard-sorting branch October 3, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants