[Emotion] Convert EuiBasicTable #6539

cee-chen · 2023-01-20T04:27:14Z

Summary

Me: Oh basic_table.scss is pretty small, I should just super quickly go in and convert this component!
Also me: Oh yeah this is going to need a new theme helper. Also I'm going to rewrite literally all the unit tests.

I strongly recommend following along by commit, and also taking a shot when you get to the unit tests commit 🤪

QA

Go to https://eui.elastic.co/pr_6539/#/tabular-content/in-memory-tables#in-memory-table-selection
Click on Load Users, confirm the loading animation works as expected/the same as prod
Click on Load Users (Error), confirm the loading animation works as expected/the same as prod

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately

Emotion conversion checklist

- which is generally a nicer API than the one I yolo'd

- I opted not to create a top-level component for this due to the very limited styles being applied, and due to HOC/theme access shenanigans

- by only rendering one `<tbody>`, not multiple

+ switch `render` to RTL

- switch to RTL totally (shallow was not handling the new render prop well) - DRY out various repeated props - stop use snapshots for every single test - use specific assertions instead. For visual rendering for various prop combos, we should use Storybook - leave snapshots in for two specific render tests - barebones & kitchen sink props

- this matches how EuiProgress behaves + clean up animation shorthand

cee-chen · 2023-01-20T04:54:22Z

src/components/basic_table/basic_table.styles.ts

+    animation: ${tableLoadingLine} 1s linear infinite;
+
+    ${euiCantAnimate} {
+      animation-duration: 2s;
+    }


[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;

}

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.

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

Sounds great, thanks @miukimiu!

- apparently `position: relative` on the parent and not on the `tbody` was a cross-browser fix :(

kibanamachine · 2023-01-20T05:14:11Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6539/

cee-chen · 2023-01-20T05:21:25Z

src/components/basic_table/basic_table.tsx

+          <EuiTableBody
+            bodyRef={this.setTbody}
+            css={loading && euiBasicTableBodyLoading(theme)}
+          >
+            {content}
+          </EuiTableBody>


This changes the error message & empty message to also set a body ref, but after investigating setTbody and what we're even using that for I have fairly strong feelings about getting rid of us blocking actions while loading is true (#5014), although I would prefer to remove all that in a separate PR.

kibanamachine · 2023-01-20T05:40:35Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6539/

cee-chen · 2023-01-23T16:46:32Z

src/services/theme/hooks.tsx

+/**
+ * Render prop alternative for complex class components
+ * Most useful for scenarios where a HOC may interfere with typing
+ */
+export const RenderWithEuiTheme = <T extends {} = {}>({
+  children,
+}: {
+  children: (theme: UseEuiTheme) => React.ReactElement;
+}) => {
+  const theme = useEuiTheme<T>();
+  return children(theme);
+};


A quick explanation for why I added a new theme util for class components instead of simply attempting to use the existing withEuiTheme HOC:

Typescript lost its mind, especially typing around ref usage (used to access internal class methods). We've run into this in the past with EuiDualRange so I sorta knew this was coming:

eui/src/components/form/range/dual_range.test.tsx

Lines 219 to 224 in 1ec1c34

// This super annoying type is now required to pass both the `ref` typing and account for instance methods

type EuiDualRangeRef = React.ComponentProps<typeof EuiDualRange> &

EuiDualRangeClass;

const ConsumerDualRange = ({ callResize }: { callResize?: boolean }) => {

const rangeRef = useRef<EuiDualRangeRef>(null);

Unfortunately, EuiBasicTable is worse than EuiDualRange because 1. it uses a typescript generic <T>, which the current withEuiTheme HOC appears to find impossible to understand (or I'm not using it correctly?), and 2. ref access to set internal selection state is a more common pattern with EuiBasicTable (we specifically document it in our table examples).

Since EuiBasicTable usage is generally higher across Kibana, I very specifically wanted to go out of my way to avoid type and snapshots headaches downstream in Kibana, by leaving its exported class component intact and grabbing the EUI theme via render prop instead. This was doable because styles on EuiBasicTable is actually very light and is limited to a conditional loading state.

cee-chen · 2023-01-23T16:51:44Z

src/components/basic_table/basic_table.tsx

+      <RenderWithEuiTheme>
+        {(theme) => (
+          <EuiTableBody
+            bodyRef={this.setTbody}
+            css={loading && euiBasicTableBodyLoading(theme)}
+          >


For context: The reason why I wrapped the loading styles around just the EuiTableBody and not the entire render()ed JSX was to reduce the previous .euiBasicTable-loading tbody {} CSS nesting to a single flat selector.

In general because the EuiBasicTable-specific styles are so light I think this worked fine. I didn't bother doing a traditional Emotion CSS-in-JS object with an euiBasicTable: css``, line because I didn't see it as necessary - we don't need to apply any styles to the actual wrapper and all the styles are conditional to the loading condition and could be represented by a simple &&.

cee-chen · 2023-01-23T16:52:51Z

src/components/basic_table/basic_table.tsx

-      {
-        'euiBasicTable-loading': loading,
-      },
+      { 'euiBasicTable-loading': loading },


Unfortunately this modifier class has 48 usages in 21 files in Kibana so I left it alone / didn't bother converting it to a data attr since usage is so high.

cee-chen · 2023-01-23T16:56:06Z

src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap

+      <tbody
+        class="css-0"
+      >


(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).

elizabetdev

Thanks, @cee-chen.

I tested in Chrome, Safari, and Firefox. I also looked at the code. LGTM! 👍🏽

Not sure if an engineer should also take a look.

elizabetdev · 2023-01-23T16:26:11Z

src/components/basic_table/basic_table.styles.ts

+    animation: ${tableLoadingLine} 1s linear infinite;
+
+    ${euiCantAnimate} {
+      animation-duration: 2s;
+    }


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.

1Copenut

👍 LGTM! I took a couple of hours to really understand what was being added and how you're wrapping complicated class components. Your tests and comments were a big help explaining the HOC and render props. Much appreciated.

cee-chen · 2023-01-23T23:34:13Z

Thanks y'all, much appreciated!

* [EuiToolTip] Add `repositionOnScroll` prop (#6515) * Add `repositionOnScroll` prop and event listener * [Docs] Add fixed tooltip example + add auto-focus behavior for keyboard users * changelog * Updated changelog. * 72.2.0 * Updated documentation. * [EuiDataGrid] Fix nested interactive controls axe error (#6517) * Fix nested interactive controls error in column visibility control * Restore mouse experience by making switch "label" draggable - this (almost) restores previous draggable functionality - the column name and handle are both mouse draggable, while the switch is not as it is its own interactive thing * Fix nested interactive controls error in column sorting control - the X button and sort button group should not be draggable as they are interactive * Improve mouse experience by making column name draggable - The x button and sort button group are interactive and cannot be draggable, but the column name/token can be + tweak padding to belong to the column name, for extra grabbable area * [opinionated] Fix multiple CSS styles to actually work correctly - whether or not these style changes are wanted will have to be approved by a designer * changelog * Improve Kibana upgrade docs (#6518) - add table of contents - order steps more generally in chronological order so it's easier to follow vs jumping around the page - update recommendations and general context * [Docs] Update `EuiConfirmModal` examples (#6519) * [Docs] Update `EuiConfirmModal` examples * Adding a width * Update src-docs/src/views/modal/confirm_modal.tsx Co-authored-by: florent-leborgne <[email protected]> Co-authored-by: florent-leborgne <[email protected]> * [EuiToolTip] Enforce only one visible tooltip at a time (#6520) * [misc cleanup] Group relative imports by general concept - keep parent services together, keep tooltip-specific imports together * Add tooltip manager that hides all other tooltips when a new tooltip is shown * Write Cypress E2E tests for multiple tooltip behavior * Changelog * Unit tests for control of internal tooltip visibility state via `ref` (#6522) * [Docs] Fix code snippet (#6526) * Added a11y specs for EuiNotificationEvent, EuiPageHeader, EuiPortal (#6524) * Added a11y specs for EuiNotificationEvent, EuiPageHeader, EuiPortal * Added a11y and keyboard specs for EuiNotificationEvent. * Added page header a11y tests. * Added a11y and keyboard specs for EuiPortal. * Simplifying Portal test, adding breadcrumb and tabs to Page Header. * Simplified two components, added a state check to EuiNotification. * Further reducing the EuiPortal specs to test just that one component. * update i18ntokens * Updated changelog. * 73.0.0 * Updated documentation. * Check that the `upstream` remote is correctly set before proceeding in release script (#6528) * [EuiModalHeaderTitle] Remove automatic detection of title contents in favor of always wrapping a `h1`, configurable via new `component` prop (#6530) * Use a `component` prop for EuiModalHeaderTitle tag wrapper instead of trying to detect child types - this is because Kibana's `<FormattedMessage>` component (which only outputs a string) is incorrectly not getting styles applied to it * Add prop unit tests + convert tests to RTL while we're here * Allow `EuiConfirmModal`s to override title component tag as well if necessary * Changelog * tweak changelog copy a bit more * [Docs EuiTable] Replace all the demos using `data_store` with `faker-js` (#6521) * Replacing `data_store` with `faker-js` * Converted `actions.js` to `tsx` * Converted `auto.js` to `.tsx` * Converted `expanding_rows.js` to `*.tsx` * Converted `footer.js` to `*.tsx` * Converted `mobile.js` to `*.tsx` * Converted `selection.js` to `*.tsx` * Converted `sorting.js` to `*.tsx` * Fix typing/`any` usages - provide correct EuiBasicTable typescript usages/definitions * Clean up divs and Fragments * Simplify & clean up delete selected items code + remove it entirely from `footer` and `expanding_rows` examples - those demos really don't need that behavior at all * Remove unnecessary `renderStatus` abstraction - not being used in multiple places, so no need to DRY it out * Fix incorrectly rendering mobile names + DRY out `renderStatus` - not totally clear to me why those examples have such complex mobile examples + add a `mobileOptions.only` example in the `mobile` demo to make up for removed examples * Remove unused `sortable` - there's no sorting being passed to the table, so it does nothing * Clean up table layout example - remove need for randomly generated group IDs - simplify var names - use value passed directly from `onChange` instead of using a `.find()` - use flex group for button toggles instead of `&emsp;`s * Clean up emoji flag logic - move `getEmojiFlag` util to bottom of the file - it's not relevant to anything table-related and shouldn't be the first thing a dev has to read/parse - remove unnecessary extra flag/countryCode() call * Remove unnecessary data length abstraction/logic - if `userLength` isn't being used anywhere else, just hard code it in - no need for a var - remove unnecessary `filteredUsers` logic + remove unnecessary `RIGHT_ALIGNMENT` import * Organize table/component logic by concept - move `columns` out to static init where logical - add headings to sort out manual pagination/sorting logic where it isn't necessarily relevant to the demo (but aids in visual output) - add comment to `findUsers` to more clearly explain what it's doing - remove unnecessary abstractions for longer files * Removed `getEmojiFlag` * Remove lodash `uniqBy` dependency - replace one instance with a `Set` example for uniqueness * Tweak new location column to truncate - this column is now fairly long compared to before - let's restore the previous table visual apperances by truncating * Standardize inconsistent link behavior - it doesn't really make sense to have a last name be a link as opposed to a username - some demos were doing this, some weren't, so standardize the rendering - remove actual external links to github - most of the newly random usernames aren't valid in any case Co-authored-by: Constance Chen <[email protected]> * Removed domain check from EuiLink (#6535) This PR changes the behavior of the EUILink component to apply rel="noreferrer" universally for all external links, as it's considered a best practice. Prior to this PR, there was an exception for elastic.co domains, added in #1565. As this behavior is no longer required, we're opting to remove the exception. * [EuiBasicTable] Fix row heights jumping when actions are disabled (#6538) * [EuiBasicTable] Fix row heights jumping when actions are disabled * Update snapshots * changelog * Adding EuiModal callout for H1 rendering. (#6497) * Adding EuiModal callout for H1 rendering. * Updating text to be more concise and clear. * Updating documentation for default H1 wrapper on EuiModalHeaderTitle. * Removing example callout, added explanation to component type definition. * Removed extra H1 tags from Docs examples. * Removed H1 from EUIModal component and a11y specs. * Update checklist template to recommend `@default` jsdoc usage (#6541) * [Emotion] Convert EuiBasicTable (#6539) * [tech debt] convert `useEuiTheme` tests to RTL `renderHook` - which is generally a nicer API than the one I yolo'd * [tech debt] Add more missing unit tests for `useEuiTheme` * [tech debt] write basic unit test for `withEuiTheme` * Add new `RenderWithEuiTheme` render prop util * Convert `tbody` loading styles to Emotion - I opted not to create a top-level component for this due to the very limited styles being applied, and due to HOC/theme access shenanigans * Fix error/empty states not rendering loading styles - by only rendering one `<tbody>`, not multiple * Write basic `loading` test + switch `render` to RTL * [extra] Massive clean up of EuiBasicTable unit tests - switch to RTL totally (shallow was not handling the new render prop well) - DRY out various repeated props - stop use snapshots for every single test - use specific assertions instead. For visual rendering for various prop combos, we should use Storybook - leave snapshots in for two specific render tests - barebones & kitchen sink props * Delete scss files * Add `shouldRenderCustomStyles` test * changelog * Add affordance for reduced motion media query - this matches how EuiProgress behaves + clean up animation shorthand * Add CSS workaround/fix for visual Safari bug - apparently `position: relative` on the parent and not on the `tbody` was a cross-browser fix :( * [EuiResizablePanel] Added tabindex prop to EuiResizablePanel for keyboard accessibility. (#6534) * Added tabindex prop to EuiResizablePanel for keyboard accessibility. * Added unit test and corrected tabIndex type. * Renaming tabindex to tabIndex for consistency in docs. * Renaming test description. Removing changelog. Co-authored-by: Jason Stoltzfus <[email protected]> Co-authored-by: Elizabet Oliveira <[email protected]> Co-authored-by: florent-leborgne <[email protected]> Co-authored-by: Trevor Pierce <[email protected]> Co-authored-by: Bree Hall <[email protected]>

## Summary `[email protected]` ⏩ `[email protected]` --- ## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1) **Bug fixes** - Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props ([#6547](elastic/eui#6547)) ## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0) - Added the `component` prop to `EuiModalHeaderTitle`, which allows overriding the default `h1` tag ([#6530](elastic/eui#6530)) - Added the `titleProps` prop to `EuiConfirmModal`, which allows overriding the default `h1` tag ([#6530](elastic/eui#6530)) **Bug fixes** - Fixed slight row height jumping in `EuiBasicTable`s when actions with tooltips became disabled ([#6538](elastic/eui#6538)) **Breaking changes** - `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag (previously attempted to conditionally detect whether its children were raw strings or not). To change this tag type to, e.g. a more generic `div`, use the new `component` prop. ([#6530](elastic/eui#6530)) - `EuiLink` now applies `rel="noreferrer"` to all domains, including `elastic.co` ([#6535](elastic/eui#6535)) - `EuiBasicTable` no longer blocks mouse/keyboard interactions while `loading` ([#6543](elastic/eui#6543)) **CSS-in-JS conversions** - Converted `EuiBasicTable` to Emotion ([#6539](elastic/eui#6539)) - Added a new `RenderWithEuiTheme` render prop utility ([#6539](elastic/eui#6539)) --------- Co-authored-by: Kibana Machine <[email protected]>

## Summary `[email protected]` ⏩ `[email protected]` --- ## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1) **Bug fixes** - Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props ([elastic#6547](elastic/eui#6547)) ## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0) - Added the `component` prop to `EuiModalHeaderTitle`, which allows overriding the default `h1` tag ([elastic#6530](elastic/eui#6530)) - Added the `titleProps` prop to `EuiConfirmModal`, which allows overriding the default `h1` tag ([elastic#6530](elastic/eui#6530)) **Bug fixes** - Fixed slight row height jumping in `EuiBasicTable`s when actions with tooltips became disabled ([elastic#6538](elastic/eui#6538)) **Breaking changes** - `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag (previously attempted to conditionally detect whether its children were raw strings or not). To change this tag type to, e.g. a more generic `div`, use the new `component` prop. ([elastic#6530](elastic/eui#6530)) - `EuiLink` now applies `rel="noreferrer"` to all domains, including `elastic.co` ([elastic#6535](elastic/eui#6535)) - `EuiBasicTable` no longer blocks mouse/keyboard interactions while `loading` ([elastic#6543](elastic/eui#6543)) **CSS-in-JS conversions** - Converted `EuiBasicTable` to Emotion ([elastic#6539](elastic/eui#6539)) - Added a new `RenderWithEuiTheme` render prop utility ([elastic#6539](elastic/eui#6539)) --------- Co-authored-by: Kibana Machine <[email protected]>

cee-chen added 10 commits January 19, 2023 20:17

[tech debt] convert useEuiTheme tests to RTL renderHook

6bcb0a2

- which is generally a nicer API than the one I yolo'd

[tech debt] Add more missing unit tests for useEuiTheme

3f5e8c1

[tech debt] write basic unit test for withEuiTheme

f396769

Add new RenderWithEuiTheme render prop util

bbe16ef

Convert tbody loading styles to Emotion

9a5d0c9

- I opted not to create a top-level component for this due to the very limited styles being applied, and due to HOC/theme access shenanigans

Fix error/empty states not rendering loading styles

3ea3cb6

- by only rendering one `<tbody>`, not multiple

Write basic loading test

f9ba5cf

+ switch `render` to RTL

Delete scss files

7563c40

Add shouldRenderCustomStyles test

dfcd39b

cee-chen added the emotion label Jan 20, 2023

cee-chen added 2 commits January 19, 2023 20:30

changelog

c6687c6

Add affordance for reduced motion media query

ea9b8d0

- this matches how EuiProgress behaves + clean up animation shorthand

cee-chen commented Jan 20, 2023

View reviewed changes

Add CSS workaround/fix for visual Safari bug

6a889eb

- apparently `position: relative` on the parent and not on the `tbody` was a cross-browser fix :(

cee-chen marked this pull request as ready for review January 20, 2023 05:19

cee-chen commented Jan 20, 2023

View reviewed changes

elizabetdev self-requested a review January 23, 2023 15:41

cee-chen commented Jan 23, 2023

View reviewed changes

elizabetdev approved these changes Jan 23, 2023

View reviewed changes

1Copenut self-requested a review January 23, 2023 19:13

1Copenut approved these changes Jan 23, 2023

View reviewed changes

cee-chen merged commit be23158 into elastic:main Jan 23, 2023

cee-chen deleted the emotion/basic-table branch January 23, 2023 23:34

This was referenced Jan 24, 2023

Upgrade EUI to v74.0.1 elastic/kibana#149457

Merged

[EuiInlineEdit] (POC Review) Create Component Directory and Base Functionality #6533

Closed

cee-chen mentioned this pull request Mar 22, 2024

[Emotion] EuiBasicTable #6387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert EuiBasicTable #6539

[Emotion] Convert EuiBasicTable #6539

cee-chen commented Jan 20, 2023 •

edited

Loading

cee-chen Jan 20, 2023 •

edited

Loading

elizabetdev Jan 23, 2023

cee-chen Jan 23, 2023

cee-chen Jan 23, 2023

kibanamachine commented Jan 20, 2023

cee-chen Jan 20, 2023

kibanamachine commented Jan 20, 2023

cee-chen Jan 23, 2023 •

edited

Loading

cee-chen Jan 23, 2023 •

edited

Loading

cee-chen Jan 23, 2023

cee-chen Jan 23, 2023 •

edited

Loading

elizabetdev left a comment

elizabetdev Jan 23, 2023

1Copenut left a comment

cee-chen commented Jan 23, 2023

	animation: ${euiIndeterminateAnimation} 1s
	${euiTheme.animation.resistance} infinite;

	${euiCantAnimate} {
	animation-duration: 2s;
	animation-timing-function: linear;
	}

	// This super annoying type is now required to pass both the `ref` typing and account for instance methods
	type EuiDualRangeRef = React.ComponentProps<typeof EuiDualRange> &
	EuiDualRangeClass;

	const ConsumerDualRange = ({ callResize }: { callResize?: boolean }) => {
	const rangeRef = useRef<EuiDualRangeRef>(null);

[Emotion] Convert EuiBasicTable #6539

[Emotion] Convert EuiBasicTable #6539

Conversation

cee-chen commented Jan 20, 2023 • edited Loading

Summary

QA

General checklist

Emotion conversion checklist

cee-chen Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

elizabetdev Jan 23, 2023

Choose a reason for hiding this comment

cee-chen Jan 23, 2023

Choose a reason for hiding this comment

cee-chen Jan 23, 2023

Choose a reason for hiding this comment

kibanamachine commented Jan 20, 2023

cee-chen Jan 20, 2023

Choose a reason for hiding this comment

kibanamachine commented Jan 20, 2023

cee-chen Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Jan 23, 2023

Choose a reason for hiding this comment

cee-chen Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

elizabetdev left a comment

Choose a reason for hiding this comment

elizabetdev Jan 23, 2023

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 23, 2023

cee-chen commented Jan 20, 2023 •

edited

Loading

cee-chen Jan 20, 2023 •

edited

Loading

cee-chen Jan 23, 2023 •

edited

Loading

cee-chen Jan 23, 2023 •

edited

Loading

cee-chen Jan 23, 2023 •

edited

Loading