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

[EuiDataGrid] Badges in grid toolbar #7369

Merged
merged 27 commits into from
Nov 21, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Nov 15, 2023

Summary

This PR changes EuiDataGrid toolbar UI to render badges for toolbar controls.

Before:
Screenshot 2023-11-15 at 17 30 04
Screenshot 2023-11-15 at 17 29 03

After:
Screenshot 2023-11-15 at 17 29 42
Screenshot 2023-11-15 at 17 29 19

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • 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
    • Updated the Figma library counterpart

@jughosta jughosta marked this pull request as ready for review November 15, 2023 18:18
@jughosta jughosta requested a review from a team as a code owner November 15, 2023 18:18
@cee-chen cee-chen self-requested a review November 15, 2023 18:19
@andreadelrio
Copy link
Contributor

@jughosta this looks great! thank you for working on this. I'll approve now with the note below for follow-up:

At some point, we're going to have to establish patterns regarding badges and counters. Michael and I will bring this up with the UX Working Group for patterns. There are at least three current use cases that are similar but we handle them differently. We should aim for alignment and consistency.

1. This PR

  • All available options (n) are selected => subdued, text shows the number of total available options.
  • Some (x) of the available options (n) are selected => subdued, x/n text.
image image

2. EuiFilterButton

  • None of the available options (n) are selected => subdued, text shows the number of total available options (n).
  • Some (x) of the available options (n) are selected => success or accent, x text.
image image

3. Discover mobile view (which as far as I can tell doesn't use EuiFilterButton, not sure why?)

  • None of the available options (n) are selected => accent, text shows 0.
  • Some (x) of the available options (n) are selected => accent, x text.
image image

@cee-chen cee-chen changed the base branch from main to datagrid-redesigns November 20, 2023 22:56
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

At a glance, these are my higher level comments/questions - the public export is the biggest question I have for you (as to whether you foresee consumers needing it). If you do plan on using it in Kibana as a consumer, great/no worries, feel free to push back on that.

I may push some quick code cleanup to this branch here before approval/merge, but they will be fairly minor. This is a really nice PR, awesome job Julia!

<EuiNotificationBadge
color="subdued"
aria-label={badgeAriaLabel}
role="marquee" // https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/marquee_role
Copy link
Member

Choose a reason for hiding this comment

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

I love this!! 🤩 Awesome call here!

src/components/datagrid/index.ts Show resolved Hide resolved
@cee-chen
Copy link
Member

Will plan on pushing up some minor cleanup and screen reader improvements today, and then merge this PR into the feature branch after Julia!

(setup for datagrid button cleanup)

- `textProps &&` is required now for typing, to avoid `false` shenanigans
- misc/optional cleanup while we're here and thinking about badges in buttons

+ update downstream `EuiSearchBar` snapshots
- this allows us to remove the extra flex group/wrappers completely, as the text and badge node now sit as siblings and automatically inherit button content flex layout

- also lets us correctly text truncate the text content, if needed
- TBH, the types are so simple I don't anticipate many consumers needing it, and I'd rather keep them where they're used. If consumers really do need it they can import directly from this file or use PropsOf

- use `FC<>` typing to match rest of EUI
- add styles for underlining text, but not the badge

- fix cursor on badge

- add `className` hooks to both text & badge nodes in case consumers want to customize their CSS

- misc/consistency - rename props (rest, classes, etc) to match other EUI components
- in favor of `.euiDataGridToolbarControl` (now that we have a specific component for this)

- remove the active `font-weight` as well, since it's no longer being used

- remove `.euiDataGrid__controlBtn` from the icon buttons as well - they're not consistently applied and they're not really doing anything, nor are they being hooked into in Kibana for the icon buttons

- replace fullscreen hook with `aria-pressed` instead

+ snapshot updates
- make unit tests more focused
- add specific assertions in addition to snapshots
- add test for new `textProps` behavior
- make the text assertion more specific, now that we can look at the badge node
- This is too noisy for a datagrid-specific PR, I'll cherry-pick it out and open up a follow-up PR once this one merges

This reverts commit 3408cfb.
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Hey Julia! The major/meaningful functionality changes I pushed up were in 184f1b7 - I didn't want the underline to apply to the badge (to match our EuiFilterButton component) and I also noticed the mouse cursor on the badge was a bit off. This should be all fixed now!

Most of the other changes are from me noticing that there was an extra text wrapper rendering on EuiButtonEmpty that should not have been (if the button children isn't text, the wrapper shouldn't appear), and that was an existing mistake in production that I've now corrected. This resulted in snapshot changes/DOM cleanups, but should otherwise not have affected the datagrid portion of this PR.

I've also opted to remove the .euiDataGrid__controlBtn className in favor of a new .euiDataGridToolbarControl class, to match the new component (f467443). I've pre-emptively looked into how this affects Kibana and feel fairly confident that the resulting changes should be easy to update in our next Kibana upgrade.

Please LMK if you have any questions! I'm going to do another quick QA pass and then merge this into the datagrid feature branch after, and then open up a PR with the final feature branch work for a final sanity check/QA from designers.

@cee-chen
Copy link
Member

cee-chen commented Nov 21, 2023

Buildkite is having outages today, hence the failing CI 🥲 gonna give this a few more hours to see if they can resolve the issues - if not, I'll force merge into the feature branch by EOD just so we can get that PR open and ready to review before holidays/travel etc

@cee-chen
Copy link
Member

buildkite test this

- gets me every time...
@cee-chen cee-chen merged commit 19b1518 into elastic:datagrid-redesigns Nov 21, 2023
3 of 6 checks passed
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@jughosta jughosta deleted the 7283-grid-toolbar-badges branch November 22, 2023 08:35
@jughosta
Copy link
Contributor Author

@cee-chen Awesome! Thanks a lot for all the changes!

cee-chen added a commit to elastic/kibana that referenced this pull request Jan 5, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([#7392](elastic/eui#7392))

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

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([elastic#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([elastic#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([elastic#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([elastic#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([elastic#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([elastic#7392](elastic/eui#7392))

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

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([elastic#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([elastic#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([elastic#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([elastic#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([elastic#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([elastic#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([elastic#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([elastic#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([elastic#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([elastic#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([elastic#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([elastic#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([elastic#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([elastic#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([elastic#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([elastic#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([elastic#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([elastic#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([elastic#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([elastic#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([elastic#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([elastic#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([elastic#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([elastic#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([elastic#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([elastic#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <[email protected]>
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.

[EuiDataGrid] Use badges for sorting and column controls in toolbar
5 participants