-
-
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
[DataGrid] Workaround for failing jsdom tests caused by :has
selectors
#14559
Conversation
…und jsdom/nwsapi bug
Deploy preview: https://deploy-preview-14559--material-ui-x.netlify.app/ |
@@ -289,12 +289,10 @@ export const GridRootStyles = styled('div', { | |||
// - the column has a left or right border | |||
// - the next column is pinned right and has a left border | |||
[`& .${c.columnHeader}:focus, | |||
& .${c.columnHeader}:focus-within, | |||
& .${c.columnHeader}:has(+ .${c.columnHeader}:focus), | |||
& .${c.columnHeader}:has(+ .${c.columnHeader}:focus-within), |
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 did not attempt to work around the focus-within
selectors, as I don't think it is worth the added JS to replicate this effect.
: false; | ||
const isLastUnpinned = | ||
columnIndex + 1 === columnPositions.length - 1 - (pinnedColumns.right.length - 1); | ||
|
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.
Should the conditions above be different for LTR and RTL?
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.
@cherniavskii Good point, though from testing this, it looks like we've got a bigger problem to solve with pinned columns + RTL 😬
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.
Looks like we have an issue open for this already #14245
I'll take a look at that next.
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.
@cherniavskii tested this with the changes from #14586 and these conditions are working as expected as they are - I don't think we need to adjust these for RTL.
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.
Could we add comments somewhere to explain those props/styles are a workaround for jsdom?
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.
Added comment here bcca78e
This PR is working around an issue with
nwsapi
where:has
selectors are not being parsed correctly.nwsapi
is a dependency ofjsdom
, so users are experiencing tests failing as a result. We can potentially undo these changes once the issue is fixed there.The temporary solution is to remove the
:has
selectors and to use modifier classes on the column headers to achieve the same result.Fixes #14517