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

[EuiListGroupItem] Add external prop #7352

Merged

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Nov 8, 2023

Summary

This PR adds an external prop to the EuiListGroupItem component so that you can mark items in an EuiListGroup as external in the same way that the EuiLink component supports:

Screenshot 2023-11-09 at 8 35 07 AM

As described in elastic/kibana#165771, we need to mark some of our EuiListGroup items as external in the new Links panel. While we could have done this in-line using the label prop (since it supports ReactNodes), this would mean we would also have to re-implement truncation support - otherwise, the popout icon would get lost when the panel gets small enough, as shown in the first link in this GIF:

Nov-09-2023 08-19-44

As you can see above, by adding the external prop to the EUI component and using that instead (the second link in the GIF), the truncation support happens without any extra work on our side 🎉

QA

General checklist

  • Browser QA
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
    • Added documentation
    • Props have proper autodocs (using @default if default values are missing)
    • Checked Code Sandbox works for any docs examples
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@Heenawter Heenawter force-pushed the euiListGroupItem_add-external-prop_2023-11-08 branch from 31c0c0b to 15cbd09 Compare November 9, 2023 15:01
@Heenawter Heenawter force-pushed the euiListGroupItem_add-external-prop_2023-11-08 branch from 15cbd09 to 0db85d0 Compare November 9, 2023 15:23
@Heenawter Heenawter marked this pull request as ready for review November 9, 2023 15:53
@Heenawter Heenawter requested a review from a team as a code owner November 9, 2023 15:53
@cee-chen cee-chen self-requested a review November 10, 2023 17:20
@cee-chen
Copy link
Member

cee-chen commented Nov 13, 2023

Hey Hannah! I'm so sorry it's taken me a couple of days to get to this PR, I had some real life shenanigans crop up. This is a really fantastic contribution, thank you so much for it! Do you care if I push up any cleanup items/feedback directly to your branch, or would you rather have a more traditional PR review process?

@Heenawter
Copy link
Contributor Author

Heenawter commented Nov 14, 2023

@cee-chen Absolutely no worries at all - life happens! ❤️

I'm totally fine if you push any cleanup commits directly, if that is easier for you 👍 I can take a look at the changes so I can understand what I could have done differently :)

- not needed for individual PRs - our release script will take care of this automatically later
- this does affect our docs props table rendering, so IMO is nice to keep together
…cator

- will be used by both EuiLink and EuiListGroupItem
+ remove `left` CSS on SR text, IMO it's not doing anything / isn't necessary - see elastic#5215 (review)
- remove icon from being rendered for `button`s - it doesn't make sense to do so as buttons should not be links. this also matches EuiLink behavior

- remove extra branching tests - the new component unit tests this sufficiently

- remove snapshots in favor of specific assertions
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.

I went ahead and refactored out a new internal subcomponent that handles both the icon and the screen reader text (which is, IMO, important!) for both EuiLink and EuiListGroupItem to use. LMK if the changes make sense to you, and I'll go ahead and merge. Thanks again for the great feature! 🎉

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@Heenawter
Copy link
Contributor Author

@cee-chen Oh, that's so clean 🤩 Yup, makes total sense - thank you so much! 🙇

@cee-chen cee-chen merged commit 7740aa3 into elastic:main Nov 15, 2023
7 checks passed
@Heenawter Heenawter deleted the euiListGroupItem_add-external-prop_2023-11-08 branch November 15, 2023 17:04
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 <julia.rechkunova@elastic.co>
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 <julia.rechkunova@elastic.co>
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