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

[DataGridPro] Fix column resizing in RTL mode #4989

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented May 24, 2022

@mui-bot
Copy link

mui-bot commented May 24, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 248.5 492.5 394.6 368.24 84.149
Sort 100k rows ms 440.4 1,071.3 720 739.62 200.152
Select 100k rows ms 139.8 215 206.1 187.96 28.539
Deselect 100k rows ms 103.5 303.5 133.4 160.6 72.667

Generated by 🚫 dangerJS against 074a34f

@cherniavskii cherniavskii added the component: data grid This is the name of the generic UI component, not the React module! label May 27, 2022
@m4theushw m4theushw self-requested a review May 30, 2022 19:11
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

I'm OK with the solution. I realized that column pinning doesn't work correctly with RTL (not because of this PR).

image

@@ -245,7 +259,7 @@ export const useGridColumnResize = (
const doc = ownerDocument(apiRef.current.rootElementRef!.current);
doc.body.style.cursor = 'col-resize';

separatorSide.current = getSeparatorSide(event.currentTarget);
separatorSide.current = getSeparatorSide(event.currentTarget, theme.direction);
Copy link
Member

Choose a reason for hiding this comment

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

Now makes sense to rename this ref to something like expansionDirection, since it doesn't represent the side of the separator anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point. Since it's not always an expansion, maybe resizeDirection?

Copy link
Member

Choose a reason for hiding this comment

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

resizeDirection is good.

@cherniavskii
Copy link
Member Author

I realized that column pinning doesn't work correctly with RTL

I've created an issue #5063 and linked it in umbrella issue for RTL #230

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Looks great

I like the resizeDirection name

@cherniavskii cherniavskii merged commit f2f04ce into mui:master Jun 1, 2022
@cherniavskii cherniavskii deleted the column-resize-rtl branch June 1, 2022 16:34
@jamesDin
Copy link

jamesDin commented Jun 6, 2022

Wokring great! Thanks guys 👍 💯

joserodolfofreitas pushed a commit to joserodolfofreitas/mui-x that referenced this pull request Jun 13, 2022
* flip column resizing logic in RTL mode

* rename separatorSide to resizeDirection
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
* flip column resizing logic in RTL mode

* rename separatorSide to resizeDirection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGrid - Resizing Columns when RTL mode
5 participants