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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] Fixed focus ring to be contained in cell #5194

Merged
merged 5 commits into from
Sep 16, 2021
Merged

[EuiDataGrid] Fixed focus ring to be contained in cell #5194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e12dfab
Fixed focus ring to be contained in cell
miukimiu Sep 16, 2021
19a8c13
Removed padding left when focused
miukimiu Sep 16, 2021
07f2bc5
Added CL
miukimiu Sep 16, 2021
22da2cc
CL again
miukimiu Sep 16, 2021
7fae3cd
Update src/components/datagrid/_mixins.scss
miukimiu Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
## [`master`](https://github.com/elastic/eui/tree/master)

No public interface changes since `38.0.0`.
**Bug fixes**

- Fixed `EuiDataGrid` focus ring to be contained in the cell ([#5194](https://github.com/elastic/eui/pull/5194))
- Fixed `EuiDataGrid` cells when focused getting a higher `z-index` which was causing long content to overlap surrounding cells ([#5194](https://github.com/elastic/eui/pull/5194))


## [`38.0.0`](https://github.com/elastic/eui/tree/v38.0.0)

Expand Down
8 changes: 4 additions & 4 deletions src-docs/src/views/datagrid/datagrid_focus_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ export const DataGridFocusExample = {
element unless a subsequent user action changes it.
</li>
<li>
Enter or F2 can be used interchangibly to enter inner cell focus
Enter or F2 can be used interchangeably to enter inner cell focus
if the logic below allows it.
</li>
</ul>
<h2>
The content and expandability of the cells dicate the focus target
The content and expandability of the cells dictate the focus target
of the cell
</h2>
<p>
Expand All @@ -70,14 +70,14 @@ export const DataGridFocusExample = {
a grid with your keyboard.
</p>
<h3>
Cell alone recieves the focus, with no possible inner focus action
Cell alone receives the focus, with no possible inner focus action
when:
</h3>
<ul>
<li>The cell is not expandable.</li>
<li>The cell has no interactive elements</li>
</ul>
<h3>A single inner element within the cell recieves focus when:</h3>
<h3>A single inner element within the cell receives focus when:</h3>
<ul>
<li>The cell is not expandable.</li>
<li>The cell has a single interaction element.</li>
Expand Down
17 changes: 1 addition & 16 deletions src/components/datagrid/_data_grid_data_row.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
position: relative;
align-items: center;
display: flex;
overflow: hidden;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

datagrid-overflow-hidden@2x

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// Hack to allow for all the focus guard stuff
> * {
Expand All @@ -30,7 +31,6 @@

&:focus {
@include euiDataGridCellFocus;
margin-top: -1px;
}

// Only add the transition effect on hover, so that it is instantaneous on focus
Expand Down Expand Up @@ -84,11 +84,6 @@
}
}

&:focus:not(:first-of-type) {
// Needed because the focus state adds a border, which needs to be subtracted from padding
padding-left: $euiDataGridCellPaddingM - 1px;
}

&.euiDataGridRowCell--numeric {
text-align: right;
}
Expand Down Expand Up @@ -253,11 +248,6 @@
@include euiDataGridStyles(paddingSmall) {
@include euiDataGridRowCell {
padding: $euiDataGridCellPaddingS;

&:focus:not(:first-of-type) {
// Needed because the focus state adds a border, which needs to be subtracted from padding
padding-left: $euiDataGridCellPaddingS - 1px;
}
}
}

Expand All @@ -272,11 +262,6 @@
@include euiDataGridStyles(paddingLarge) {
@include euiDataGridRowCell {
padding: $euiDataGridCellPaddingL;

&:focus:not(:first-of-type) {
// Needed because the focus state adds a border, which needs to be subtracted from padding
padding-left: $euiDataGridCellPaddingL - 1px;
}
}
}

Expand Down
21 changes: 17 additions & 4 deletions src/components/datagrid/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,24 @@ $euiDataGridStyles: (
}

@mixin euiDataGridCellFocus {
border: 1px solid transparent;
box-shadow: 0 0 0 2px $euiFocusRingColor;
border-radius: 1px;
z-index: 2; // Needed so it sits above potential striping in the next row
outline: none; // Remove outline as we're handling it manually

// We don't want to use a border on the focused cell directly because we want to maintain the light gray borders
// We can't use a box shadow or outline because it would be contained outside the cell and it would be cut by other surrounding cells
// So the solution is to use use a pseudo-element. It allows us to use a border, and it can be contained inside the cell by positioning it with an absolute position.
&::after {
content: '';
Comment on lines +59 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

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. 👍

Copy link
Member

Choose a reason for hiding this comment

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

++ 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Ahh right, makes sense if we want that border-radius!

display: block;
width: 100%;
height: 100%;
position: absolute;
top: 0;
left: 0;
border: $euiBorderWidthThick solid $euiFocusRingColor;
border-radius: $euiBorderRadiusSmall;
z-index: 2; // We want this to be on top of all the content
pointer-events: none; // Because we put it with a higher z-index we don't want to make it clickable this way we allow selecting the content behind
}
}

@mixin euiDataGridRowCell {
Expand Down