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

[DataGrid] Fix GridPanelAnchor positioning #15022

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Conversation

k-rajat19
Copy link
Contributor

@k-rajat19 k-rajat19 commented Oct 18, 2024

@k-rajat19 k-rajat19 changed the title [Data Grid] Fix GridPanelAnchor width [DataGrid] Fix GridPanelAnchor width Oct 18, 2024
@mui-bot
Copy link

mui-bot commented Oct 18, 2024

Deploy preview: https://deploy-preview-15022--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 971a563

@k-rajat19 k-rajat19 changed the title [DataGrid] Fix GridPanelAnchor width [DataGrid] Fix GridPanelAnchor positioning Oct 18, 2024
@k-rajat19 k-rajat19 marked this pull request as ready for review October 18, 2024 21:12
@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work regression A bug, but worse labels Oct 19, 2024
@oliviertassinari
Copy link
Member

The change makes sense, but if we look at the root, it seems strange that we need this gridPanelAnchor DOM node. I wonder if we couldn't remove it and instead rely on an existing DOM, e.g. .MuiDataGrid-topContainer.

@k-rajat19
Copy link
Contributor Author

k-rajat19 commented Oct 20, 2024

Hi @oliviertassinari ,
this approach has been decided to use here before that we relied on column header DOM which is inside the top container.
and I feel like we should stick to the current approach, though I haven't given any try to whether we can get rid of this extra element or not.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2024

this approach has been decided to use here

Ok, so the child .MuiDataGrid-columnHeaders doesn't work. Now, why would its parent .MuiDataGrid-topContainer not work?

SCR-20241020-segk

@oliviertassinari oliviertassinari mentioned this pull request Oct 20, 2024
9 tasks
@k-rajat19
Copy link
Contributor Author

k-rajat19 commented Oct 21, 2024

On surface it looks like this approach was chosen because of the positioning issues of the preference panel
if we reference panel to the top container, then in case of horizontal scroll panel will be scrolled also and becomes invisible.
@romgrk can you confirm this? if you still remember the details since the change was made more than one year ago 😅

@cherniavskii
Copy link
Member

@k-rajat19 Correct – topContainer didn't work, that's why I added panel anchor:

@cherniavskii cherniavskii removed the bug 🐛 Something doesn't work label Oct 21, 2024
@cherniavskii cherniavskii merged commit 922c8d0 into mui:master Oct 21, 2024
25 checks passed
@k-rajat19 k-rajat19 deleted the panel branch October 21, 2024 10:23
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 21, 2024
@oliviertassinari
Copy link
Member

Ok, thanks, so nothing else can be used here, nice regression fix 👍.


Off-topic. One thing we could consider in the future is to change the anchor of the filter panel bested on the trigger. There is quite a long distance for the mouse to travel: https://mui.com/x/react-data-grid/filtering/

Screen.Recording.2024-10-21.at.16.10.17.mov

Related to #6419.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGridPro placement panel filter no works
4 participants