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.

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

[EuiTable] New responsiveBreakpoint prop + initial setup for mobile vs desktop styles #7625

Merged
merged 11 commits into from
Mar 27, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 26, 2024

Summary

NOTE: This is going into the EuiTable Emotion conversion/cleanup feature branch.

This PR sets up a new responsiveBreakpoint prop for all table components (EuiTable, EuiBasicTable, and EuiInMemoryTable that allows consumers to customize the breakpoint at which tables collapse, and also cascades the dynamic responsive/mobile state down to all child cells/rows via context.

This PR only very minimally converts Sass responsive styles to Emotion - it's primarily just set up and JS proof-of-concept for now to help make PR review more bite-sized.

ℹ️ The line diff #s look, scary but it's 80% indentation from snapshots - make sure you view with whitespace changes off. As always, I also strongly recommend reviewing by commit.

QA

PLEASE NOTE that staging / responsive tables will look broken until all responsive EuiTable styles have been converted.

As such, this is mostly code review / it's probably not worth deeply QAing other than confirming the new docs responsive table true/false toggle works at all window sizes.

General checklist

  • Browser QA
    • Checked in mobile
      - [ ] Checked in both light and dark modes
      - [ ] Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

@cee-chen cee-chen changed the title [EuiTable] Convert responsive mobile vs desktop styles [EuiTable] Initial set up for responsive mobile vs desktop styles Mar 27, 2024
@cee-chen cee-chen changed the title [EuiTable] Initial set up for responsive mobile vs desktop styles [EuiTable] Initial setup for responsive mobile vs desktop styles Mar 27, 2024
@cee-chen cee-chen force-pushed the emotion/table-3 branch 2 times, most recently from 5db32d8 to d99a2f3 Compare March 27, 2024 03:46
@cee-chen cee-chen changed the title [EuiTable] Initial setup for responsive mobile vs desktop styles [EuiTable] New responsiveBreakpoint prop + initial setup for mobile vs desktop styles Mar 27, 2024
…ponsive behavior

- instead of having it be hardcoded in via CSS media queries
NOTE: docs example will look broken until all styles are fully converted to Emotion
- switch to conditionally rendering JSX instead of applying `display: none` CSS
+ convert styles to Emotion

(basic table/memory tables)
+ remove unnecessary `EuiFlexGroup`/`EuiFlexItem` components, just use flex CSS instead
+ fix InMemoryTable tests that relied on the mobile header DOM
+ add new test/snapshot
- fairly straightforward, no theme tokens needed

+ extend types
- so any child can pick up whether or not the table is responsive
+ remove unnecessary variable definitions in favor of inline JSX (microperf)
@cee-chen cee-chen marked this pull request as ready for review March 27, 2024 04:03
@cee-chen cee-chen requested a review from a team as a code owner March 27, 2024 04:03
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

Not sure if that's expected right now but there is an error when loading the staging docs site for Tables (it's working fine locally).

Screenshot 2024-03-27 at 13 16 55

@cee-chen
Copy link
Contributor Author

Not sure if that's expected right now but there is an error when loading the staging docs site for Tables (it's working fine locally).

Dangit, I was really hoping that would fix itself on second deploy/overnight. I have no idea what's going on, I can't repro it locally either 😕

@cee-chen
Copy link
Contributor Author

The style memoization error should be resolved by #7626 🤞 As that fix is getting merged into main, I'm going to go ahead and vote for skipping staging testing for now for this PR.

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ LGTM!

@cee-chen
Copy link
Contributor Author

Thanks a million!! 🙇

@cee-chen cee-chen merged commit 7cf51fc into elastic:tableflip Mar 27, 2024
3 of 6 checks passed
@cee-chen cee-chen deleted the emotion/table-3 branch March 27, 2024 18:47
@cee-chen
Copy link
Contributor Author

Lol fuck I always forget you can't auto-wait for CI on feature branch PRs

@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 27, 2024

💔 Build Failed

Failed CI Steps

History

@kibanamachine
Copy link

Preview staging links for this PR:

@cee-chen cee-chen mentioned this pull request Mar 27, 2024
7 tasks
cee-chen added a commit that referenced this pull request Mar 28, 2024
cee-chen added a commit that referenced this pull request Apr 1, 2024
cee-chen added a commit that referenced this pull request Apr 5, 2024
mistic pushed a commit to elastic/kibana that referenced this pull request Apr 18, 2024
`v93.6.0` ⏩ `v94.1.0`

> [!important]
> 👋 Hello everyone - this is a special release containing `EuiTable`'s
conversion to Emotion, several long-overdue cleanups and breaking
changes, and one or two fun new features. First, let's address the big
questions:

### Q: I'm listed as a codeowner, how much should I manually QA/review?

Answer: It depends on what exactly in your code changed, but _in
general_ I would strongly suggest at least pulling this branch down and
doing a quick visual smoke test of all tables (_note: **not**
datagrids_) in your apps/plugins. You should not expect to see any major
visual regressions.

If your table contained any kind of custom styling or behavior (e.g.
custom CSS, etc.) I **strongly** recommend taking more time to QA
thoroughly to ensure your table still looks and behaves as expected.
Teams with very manual or specific updates will be flagged below in
comment threads.

### Q: When do I need to review by?

This PR will be merged **after** 8.14FF. Because this upgrade touches so
many files and teams, we're aiming for asking for an admin merge by EOD
4/18 regardless of full approval status.

As always, you're welcome to ping us after merge if you find any issues
later ([see our
FAQ](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)),
as you will have until 8.15FF to catch any bugs.

### Q: What breaking changes were made, and why?

Here's a quick shortlist of all the changes made that affected the
majority of the commits in this PR:

#### <u>Removed several top-level table props</u>
- The `responsive` prop has been removed in favor of the new
`responsiveBreakpoint` prop (same `false` behavior as before)
- The following props were removed from basic and in-memory tables:
  - `hasActions`, `isSelectable`, and `isExpandable`
- These props were not used for functionality and were only used for
styling tables in mobile/responsive views, which is not a best practice
pattern we wanted for our APIs. Mobile tables are now styled correctly
without needing consumers to pass these extra props.
  - `textOnly`
- This prop was unused and had no meaningful impact on tables or table
content.

#### Removed hidden mobile vs. desktop DOM

Previously, EUI would set classes that applied `display: none` CSS for
content that was hidden for mobile vs. desktop. This is no longer the
case, and content that only displays for mobile or only displays for
desktop will no longer render to the DOM at all if the table is not in
that responsive state.

This is both more performant when rendering large quantities of
cells/content, and simpler to write test assertions for when comparing
what the user actually sees vs. what the DOM ‘sees’.
(c3eeb08441e4b6efe6505e7cddaa87b540ddb259,
78cefcd954a7b46eaccd05e431b5e24dc86071a3)

#### Removed direct usages of table `className`s

EuiTable `classNames` no longer have any styles attached to them, so
some instances where Kibana usages were applying the `className` for
styles have been replaced with direct component usage or removed
entirely (86ce80b).

#### Custom table cell styles

Any custom CSS for table cells was previously being applied to the inner
`div.euiTableCellContent` wrapper. As of this latest release, the
`className` and `css` props will now be applied directly to the outer
`td.euiTableRowCell` element. If you were targeting custom styles table
cells, please re-QA your styles to ensure everything still looks as
expected.

---

<details open><summary>Full changelog (click to collapse)</summary>

##
[`v94.1.0-backport.0`](https://github.com/elastic/eui/releases/v94.1.0-backport.0)

**This is a backport release only intended for use by Kibana.**

**Bug fixes**

- Fixed a visual text alignment regression in `EuiTableRowCell`s with
the `row` header scope
([#7681](elastic/eui#7681))

**Accessibility**

- Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes
to have unique aria-labels per row
([#7672](elastic/eui#7672))

## [`v94.1.0`](https://github.com/elastic/eui/releases/v94.1.0)

- Updated `EuiTableHeaderCell` to show a subdued `sortable` icon for
columns that are not currently sorted but can be
([#7656](elastic/eui#7656))
- Updated `EuiBasicTable` and `EuiInMemoryTable`'s
`columns[].actions[]`'s to pass back click events to `onClick` callbacks
as the second callback
([#7667](elastic/eui#7667))

## [`v94.0.0`](https://github.com/elastic/eui/releases/v94.0.0)

- Updated `EuiTable`, `EuiBasicTable`, and `EuiInMemoryTable` with a new
`responsiveBreakpoint` prop, which allows customizing the point at which
the table collapses into a mobile-friendly view with cards
([#7625](elastic/eui#7625))
- Updated `EuiProvider`'s `componentDefaults` prop to allow configuring
`EuiTable.responsiveBreakpoint`
([#7625](elastic/eui#7625))

**Bug fixes**

- `EuiBasicTable` & `EuiInMemoryTable` `isPrimary` actions are now
correctly shown on mobile views
([#7640](elastic/eui#7640))
- Table `mobileOptions`:
([#7642](elastic/eui#7642))
- `mobileOptions.align` is now respected instead of all cells being
forced to left alignment
- `textTruncate` and `textOnly` are now respected even if a `render`
function is not passed

**Breaking changes**

- Removed unused `EuiTableHeaderButton` component
([#7621](elastic/eui#7621))
- Removed the `responsive` prop from `EuiTable`, `EuiBasicTable`, and
`EuiInMemoryTable`. Use the new `responsiveBreakpoint` prop instead
([#7625](elastic/eui#7625))
- The following props are no longer needed by `EuiBasicTable` or
`EuiInMemoryTable` for responsive table behavior to work correctly, and
can be removed: ([#7632](elastic/eui#7632))
  - `isSelectable`
  - `isExpandable`
  - `hasActions`
- Removed the `showOnHover` prop from `EuiTableRowCell` /
`EuiBasicTable`/`EuiInMemoryTable`'s `columns` API. Use the new actions
`columns[].actions[].showOnHover` API instead.
([#7640](elastic/eui#7640))
- Removed top-level `textOnly` prop from `EuiBasicTable` and
`EuiInMemoryTable`. Use `columns[].textOnly` instead.
([#7642](elastic/eui#7642))

**DOM changes**

- `EuiTable` mobile headers no longer render in the DOM when not visible
(previously rendered with `display: none`). This may affect DOM testing
assertions. ([#7625](elastic/eui#7625))
- `EuiTableRowCell` now applies passed `className`s to the parent `<td>`
element, instead of to the inner cell content `<div>`.
([#7631](elastic/eui#7631))
- `EuiTableRow`s rendered by basic and memory tables now only render a
`.euiTableRow-isSelectable` className if the selection checkbox is not
disabled ([#7632](elastic/eui#7632))
- `EuiTableRowCell`s with `textOnly` set to `false` will no longer
attempt to apply the `.euiTableCellContent__text` className to child
elements. ([#7641](elastic/eui#7641))
- `EuiTableRowCell` no longer renders mobile headers to the DOM unless
the current table is displaying its responsive view.
([#7642](elastic/eui#7642))
- `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in
the DOM at all on mobile if their columns' `mobileOptions.show` is set
to `false`. ([#7642](elastic/eui#7642))
- `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in
the DOM at all on desktop if their columns' `mobileOptions.only` is set
to `true`. ([#7642](elastic/eui#7642))

**CSS-in-JS conversions**

- Converted `EuiTable`, `EuiTableRow`, `EuiTableRowCell`, and all other
table subcomponents to Emotion
([#7654](elastic/eui#7654))
- Removed the following `EuiTable` Sass variables:
([#7654](elastic/eui#7654))
  - `$euiTableCellContentPadding`
  - `$euiTableCellContentPaddingCompressed`
  - `$euiTableCellCheckboxWidth`
  - `$euiTableHoverColor`
  - `$euiTableSelectedColor`
  - `$euiTableHoverSelectedColor`
  - `$euiTableActionsBorderColor`
  - `$euiTableHoverClickableColor`
  - `$euiTableFocusClickableColor`
- Removed the following `EuiTable` Sass mixins:
([#7654](elastic/eui#7654))
  - `euiTableActionsBackgroundMobile`
  - `euiTableCellCheckbox`
  - `euiTableCell`
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants