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

[Emotion] Convert EuiBasicTable #6539

Merged
merged 13 commits into from
Jan 23, 2023
Merged

[Emotion] Convert EuiBasicTable #6539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6bcb0a2
[tech debt] convert `useEuiTheme` tests to RTL `renderHook`
cee-chen Jan 19, 2023
3f5e8c1
[tech debt] Add more missing unit tests for `useEuiTheme`
cee-chen Jan 19, 2023
f396769
[tech debt] write basic unit test for `withEuiTheme`
cee-chen Jan 19, 2023
bbe16ef
Add new `RenderWithEuiTheme` render prop util
cee-chen Jan 19, 2023
9a5d0c9
Convert `tbody` loading styles to Emotion
cee-chen Jan 19, 2023
3ea3cb6
Fix error/empty states not rendering loading styles
cee-chen Jan 20, 2023
f9ba5cf
Write basic `loading` test
cee-chen Jan 20, 2023
3b6e129
[extra] Massive clean up of EuiBasicTable unit tests
cee-chen Jan 20, 2023
7563c40
Delete scss files
cee-chen Jan 20, 2023
dfcd39b
Add `shouldRenderCustomStyles` test
cee-chen Jan 20, 2023
c6687c6
changelog
cee-chen Jan 20, 2023
ea9b8d0
Add affordance for reduced motion media query
cee-chen Jan 20, 2023
6a889eb
Add CSS workaround/fix for visual Safari bug
cee-chen Jan 20, 2023
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
4,982 changes: 707 additions & 4,275 deletions src/components/basic_table/__snapshots__/basic_table.test.tsx.snap
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 8 additions & 4 deletions src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ exports[`EuiInMemoryTable behavior pagination 1`] = `
</div>
</div>
<table
class="euiTable euiTable--responsive"
class="euiTable css-0 euiTable--responsive"
id="__table_generated-id"
tabindex="-1"
>
Expand Down Expand Up @@ -55,7 +55,9 @@ exports[`EuiInMemoryTable behavior pagination 1`] = `
</th>
</tr>
</thead>
<tbody>
<tbody
class="css-0"
>
<tr
class="euiTableRow"
>
Expand Down Expand Up @@ -651,7 +653,7 @@ exports[`EuiInMemoryTable with pagination and "show all" page size 1`] = `
</div>
</div>
<table
class="euiTable euiTable--responsive"
class="euiTable css-0 euiTable--responsive"
id="__table_generated-id"
tabindex="-1"
>
Expand Down Expand Up @@ -684,7 +686,9 @@ exports[`EuiInMemoryTable with pagination and "show all" page size 1`] = `
</th>
</tr>
</thead>
<tbody>
<tbody
class="css-0"
>
Comment on lines +689 to +691
Copy link
Member Author

@cee-chen cee-chen Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(Follow up to #6539 (review))

Quick FYI as to where these classes are coming from - the conditional Emotion styles I added via loading && are working correctly, but for reasons (emotion-js/emotion#1529) Emotion still applies a blank/empty css-0 class when anything is passed to the css={} prop, even false or undefined, so this gets output.

This will probably end up looking less strange when we convert all our underlying table styles to Emotion (it will get the euiTable- prefix at that point).

<tr
class="euiTableRow"
>
Expand Down
41 changes: 0 additions & 41 deletions src/components/basic_table/_basic_table.scss
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/components/basic_table/_index.scss
View file
Open in desktop

This file was deleted.

52 changes: 51 additions & 1 deletion src/components/basic_table/basic_table.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,57 @@
* Side Public License, v 1.
*/

import { css } from '@emotion/react';
import { css, keyframes } from '@emotion/react';

import { logicalCSS, euiCantAnimate } from '../../global_styling';
import { UseEuiTheme } from '../../services';

const tableLoadingLine = keyframes`
from {
${logicalCSS('left', 0)}
${logicalCSS('width', 0)}
}

20% {
${logicalCSS('left', 0)}
${logicalCSS('width', '40%')}
}

80% {
${logicalCSS('left', '60%')}
${logicalCSS('width', '40%')}
}

100% {
${logicalCSS('left', '100%')}
${logicalCSS('width', 0)}
}
`;

export const euiBasicTableBodyLoading = ({ euiTheme }: UseEuiTheme) => css`
position: relative;
overflow: hidden;

&::before {
position: absolute;
content: '';
${logicalCSS('width', '100%')}
${logicalCSS('height', euiTheme.border.width.thick)}
background-color: ${euiTheme.colors.primary};
animation: ${tableLoadingLine} 1s linear infinite;

${euiCantAnimate} {
animation-duration: 2s;
}
Comment on lines +46 to +50
Copy link
Member Author

@cee-chen cee-chen Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

[not sure if this conversation belongs in this PR or as a separate/follow-up item]

@miukimiu I'm not sure if this needs to be DRY'd out with EuiProgress at all (including the animation)? The animation is remarkably similar (but not quite the same).

https://elastic.github.io/eui/#/display/progress

eui/src/components/progress/progress.styles.ts

Lines 124 to 130 in 4c285cd

animation: ${euiIndeterminateAnimation} 1s
${euiTheme.animation.resistance} infinite;
${euiCantAnimate} {
animation-duration: 2s;
animation-timing-function: linear;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to work next on tables empty and loading states. So I think for now we can leave it as it is. Then according to the direction the design takes, I can refactor the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I do still want to investigate this, but I'd prefer we address it in a follow-up PR rather than this PR. I'm hoping to get this in by tomorrow's release so we have an Emotion conversion in for the next Kibana upgrade

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great, thanks @miukimiu!

}
`;

// Fix to make the loading indicator position correctly in Safari
// For whatever annoying reason, Safari doesn't respect `position: relative;`
// on `tbody` without `position: relative` on the parent `table`
export const safariLoadingWorkaround = () => css`
position: relative;
`;

// Unsets the extra height caused by tooltip/popover wrappers around table action buttons
// Without this, the row height jumps whenever actions are disabled
Expand Down
Loading