[EuiDataGrid] Fixed focus ring to be contained in cell #5194
Conversation
| position: relative; | ||
| align-items: center; | ||
| display: flex; | ||
| overflow: hidden; |
There was a problem hiding this comment.
I tested and I don't see a problem of having this overflow: hidden; for .euiDataGridRowCell. Am I missing something? Does it impact any use case?
In practice having it or not having it is almost the same. But the focus ring now that is contained looks a little bit better with the overflow: hidden:
There was a problem hiding this comment.
From what I could tell, the cell contents itself already has overflow: hidden so I doubt this should have an impact. It certainly should not with the normal/default settings.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5194/ |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
👍 LGTM! I tested in Chrome, FF & Safari (there were some other quirks but not related to this PR). So glad to have this fixed!
| &::after { | ||
| content: ''; |
There was a problem hiding this comment.
At first I was worried about adding a pseudo element because I was told that extra elements can impact the performance, but because this is only add on focus it doesn't seem to have an impact. 👍
There was a problem hiding this comment.
++ that I'm not overly concerned about the performance and I think this is just fine, but wanted to add that we can use an inset box-shadow to get a border inside (vs. outside) the cell if it ever becomes a perf issue: CodeSandbox demo
There was a problem hiding this comment.
Thanks @constancecchen! 🎉
At some point, I added the inset box-shadow to the cell. But then I couldn't use a border-radius because the cell has some light gray non-rounded borders. Then I just decided to add a pseudo-element. So this way we can have a rounded focus ring inside the non-rounded cell. 😅
There was a problem hiding this comment.
Ahh right, makes sense if we want that border-radius!
| position: relative; | ||
| align-items: center; | ||
| display: flex; | ||
| overflow: hidden; |
There was a problem hiding this comment.
From what I could tell, the cell contents itself already has overflow: hidden so I doubt this should have an impact. It certainly should not with the normal/default settings.
Co-authored-by: Caroline Horn <[email protected]>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5194/ |
4 participants
Summary
This PR fixes the EuiDataGrid focus ring to be contained in the cell. It also fixes cells when focused getting a higher
z-indexwhich was causing long content to overlap surrounding cells.The issue with the focus states was found on #4958.
Why contained focus rings?
As we can see having the focus rings outside of the cells was making them getting cut by other surrounding cells. IMO it was not looking good.
Checklist
[ ] Checked in mobile[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Added or updated jest tests[ ] Checked for breaking changes and labeled appropriately