[EuiCallOut] Add `onDismiss` prop #7156

kyrspl · 2023-09-05T09:55:31Z

Summary

Adds a cross button icon to the top right hand corner of the callout, which enables consumers to add dismissible callouts with strong accessible and design affordance.

QA

https://eui.elastic.co/pr_7156/#/display/callout#dismissible-callouts

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs (using @default if default values are missing) and playground toggles
Added documentation
Added or updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~

ryankeairns · 2023-09-05T15:54:37Z

Thanks @kyrspl

The refactoring raises a flag. Are you able to scale this back and remove those changes from the PR? Put another way, what is the simplest solution here?

If you feel it is absolutely necessary work, then we may consider punting on this PR and leaving it to an engineer.

kyrspl · 2023-09-05T16:59:43Z

Update -> Given the complexity of managing actions in the callout, we decided to reduce the scope of this PR to only render the close icon on the top right of the callout.

Any additional work for the Dismiss button should take place in a future PR.

src/components/call_out/call_out.tsx

src/components/call_out/call_out.styles.ts

Co-authored-by: Bree Hall <[email protected]>

src/components/call_out/call_out.tsx

Switch from `close` copy to `dismiss`, which better matches other similar components (e.g. EuiToast uses `dismissToast`)

- Dismiss button should to adjust to callout size/padding - Fix padding-right logic on top-most title or child paragraph via CSS selectors - Fix non-working spacer logic (just use a JS conditional to render a spacer instead of CSS `:not:only-child`) - Fix button DOM order for screen reader users - misc `&&` vs ternary cleanup

src/components/call_out/call_out.tsx

cee-chen · 2023-09-23T06:36:07Z

src/components/call_out/call_out.tsx

+    // Note: the DOM position of the dismiss button matters to screen reader users.
+    // We generally want them to have some context of _what_ they're dismissing,
+    // instead of navigating to the dismiss button first before the callout content
+    const calloutContent =
+      header && optionalChildren ? (
+        <>
+          {header}
+          {dismissButton}
+          <EuiSpacer size="s" />
+          {optionalChildren}
+        </>
+      ) : (
+        <>
+          {header || optionalChildren}
+          {dismissButton}
+        </>


@1Copenut Would also like your thoughts on this from a screen reader UX perspective. You can test the render orders on https://eui.elastic.co/pr_7156/#/display/callout#dismissible-callouts.

My logic here was trying to keep the DOM render order as similar to the visual order as possible. This render order allows SR users to get a high level of the callout and opt to dismiss it before navigating to the child content. Do you feel that makes sense to you?

@cee-chen Yes I do feel this order is the best approach. It feels natural to let users hear the title, then the option to dismiss the callout or hear more information.

Eventually I'd like to move the EuiModal to use this same DOM order. Right now at least the confirm modal places the close button before the title. It was a pattern that didn't feel quite right, but didn't have a champion. Now that we're revisiting with EuiCallout, I think it's worth a second discussion.

That's a good point about EuiModals (and likely flyouts as well). Probably would be worth opening a separate issue about!

Agreed. I'll open an issue now and cross-link it.

cee-chen · 2023-09-23T06:41:41Z

@kyrspl As a heads up, this is almost done & I think we can get it in for next week's release, I'm just waiting to get screen reader expertise from Trevor first before merging.

1Copenut

Left three comments, and two small suggested changes. I'll change my comment to approved after the callout accessible label is updated.

src-docs/src/views/call_out/on_dismiss.tsx

src/components/call_out/call_out.tsx

1Copenut · 2023-09-25T17:17:08Z

src/components/call_out/call_out.tsx

+    // Note: the DOM position of the dismiss button matters to screen reader users.
+    // We generally want them to have some context of _what_ they're dismissing,
+    // instead of navigating to the dismiss button first before the callout content
+    const calloutContent =
+      header && optionalChildren ? (
+        <>
+          {header}
+          {dismissButton}
+          <EuiSpacer size="s" />
+          {optionalChildren}
+        </>
+      ) : (
+        <>
+          {header || optionalChildren}
+          {dismissButton}
+        </>


@cee-chen Yes I do feel this order is the best approach. It feels natural to let users hear the title, then the option to dismiss the callout or hear more information.

Eventually I'd like to move the EuiModal to use this same DOM order. Right now at least the confirm modal places the close button before the title. It was a pattern that didn't feel quite right, but didn't have a champion. Now that we're revisiting with EuiCallout, I think it's worth a second discussion.

1Copenut

👍 LGTM! Tested the inflection of "callout" in NVDA and JAWS too.

kibanamachine · 2023-09-25T18:05:11Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7156/
Storybook: https://eui.elastic.co/pr_7156/storybook

elasticmachine · 2023-09-25T18:09:34Z

💚 Build Succeeded

Buildkite Build
Commit: 0bc10f0

History

💚 Build #699 succeeded 7fe66b3
💔 Build #696 failed b76bdbc
💔 Build #644 failed e05e1d7
💚 Build #558 succeeded 83003c0
💔 Build #520 failed c818071

`v88.3.0`⏩`v88.5.0` closes #151514 --- ## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0) - Updated `EuiCallOut` with a new `onDismiss` prop ([#7156](elastic/eui#7156)) - Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows custom rendering of the toolbar. ([#7190](elastic/eui#7190)) - Added a new `allowResetButton` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows hiding the "Reset to default" button from the display settings popover. ([#7190](elastic/eui#7190)) - Added a new `additionalDisplaySettings` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows rendering extra settings inside the display settings popover. ([#7190](elastic/eui#7190)) - Updated `EuiDataGrid`'s toolbar display settings button icon ([#7190](elastic/eui#7190)) - Updated `EuiTextTruncate` with significantly improved iteration performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation` now only uses more performant canvas render API ([#7210](elastic/eui#7210)) - Updated `EuiPopover` with a new configurable `repositionToCrossAxis` prop ([#7211](elastic/eui#7211)) - Updated `EuiDatePicker` to support `compressed` input styling ([#7218](elastic/eui#7218)) - Added `gradient` and `palette` icon glyphs. ([#7220](elastic/eui#7220)) **Bug fixes** - Fixed `EuiPopover`'s missing animations on popover close ([#7211](elastic/eui#7211)) - Fixed `EuiInputPopover` anchoring to the wrong side and missing shadows on smaller screens ([#7211](elastic/eui#7211)) - Fixed `EuiSuperDatePicker` icon spacing on the quick select button ([#7217](elastic/eui#7217)) - Fixed a missing type in `EuiMarkdownEditor`'s default processing plugins ([#7221](elastic/eui#7221)) ## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1) **Bug fixes** - Fixed missing `className`s on `EuiTextTruncate` ([#7212](elastic/eui#7212)) - Fixed `title`s on `EuiComboBox` dropdown options to always be present ([#7212](elastic/eui#7212)) - Fixed `EuiComboBox` truncation issues when search is an empty space ([#7212](elastic/eui#7212)) ## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0) - Updated `EuiComboBox` to allow configuring text truncation behavior via `truncationProps`. These props can be set on the entire combobox as well as on on individual dropdown options. ([#7028](elastic/eui#7028)) - Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to `eql`). When setting this prop to `text`, the built-in search bar will ignore EQL syntax and allow searching for plain strings with special characters and symbols. ([#7175](elastic/eui#7175)) **Bug fixes** - `EuiComboBox` now always shows the highlighted search text, even on truncated text ([#7028](elastic/eui#7028)) - Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text ([#7175](elastic/eui#7175)) - Fixed the inline compressed styles of `EuiDescriptionListTitle` to use a taller line-height for readability ([#7185](elastic/eui#7185)) - Fixed `EuiComboBox` to correctly truncate selected items when displayed as pills and plain text ([#7193](elastic/eui#7193)) **Accessibility** - Added `aria-current` attribute to `EuiTablePagination` ([#7186](elastic/eui#7186)) **CSS-in-JS conversions** - Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed `$euiDragAndDropSpacing` Sass variables ([#7187](elastic/eui#7187)) --------- Co-authored-by: Patryk Kopycinski <[email protected]> Co-authored-by: Jan Monschke <[email protected]> Co-authored-by: Thomas Watson <[email protected]>

kyrspl added 2 commits September 5, 2023 11:23

add dismissible behavior

e818cfa

add isDismissible

a055e79

kyrspl mentioned this pull request Sep 5, 2023

[EuiCallOut] Add optional close button #7038

Closed

refactor

bbf3fa8

fix title spacing

0c29f60

kyrspl added 3 commits September 5, 2023 19:05

remove dismissble button logic

ffff086

remove css declaration

8560710

refactor

c818071

kyrspl changed the title ~~[DRAFT] Callout~~ DRAFT [euiCallout] Add isDismissible prop Sep 7, 2023

kyrspl added 2 commits September 7, 2023 12:00

add ignore statement

fab246e

update onClose type and header end style

83003c0

breehall reviewed Sep 8, 2023

View reviewed changes

src/components/call_out/call_out.tsx Outdated Show resolved Hide resolved

breehall reviewed Sep 8, 2023

View reviewed changes

src/components/call_out/call_out.styles.ts Outdated Show resolved Hide resolved

breehall reviewed Sep 8, 2023

View reviewed changes

src/components/call_out/call_out.styles.ts Outdated Show resolved Hide resolved

breehall reviewed Sep 8, 2023

View reviewed changes

src/components/call_out/call_out.styles.ts Outdated Show resolved Hide resolved

Update src/components/call_out/call_out.tsx

0533b24

Co-authored-by: Bree Hall <[email protected]>

kyrspl commented Sep 19, 2023

View reviewed changes

src/components/call_out/call_out.tsx Outdated Show resolved Hide resolved

Update src/components/call_out/call_out.tsx

ed63118

kyrspl commented Sep 19, 2023

View reviewed changes

src/components/call_out/call_out.tsx Outdated Show resolved Hide resolved

kyrspl and others added 2 commits September 19, 2023 17:45

Update src/components/call_out/call_out.tsx

e05e1d7

Merge branch 'main' into callout

220ef7c

cee-chen changed the title ~~DRAFT [euiCallout] Add isDismissible prop~~ [EuiCallOut] Add onDismiss prop Sep 23, 2023

cee-chen requested a review from a team as a code owner September 23, 2023 01:11

cee-chen force-pushed the callout branch 4 times, most recently from 08337a2 to 2525a99 Compare September 23, 2023 06:28

[PR feedback] Prop name/copy

5bd9446

Switch from `close` copy to `dismiss`, which better matches other similar components (e.g. EuiToast uses `dismissToast`)

cee-chen added 5 commits September 22, 2023 23:31

Add Jest unit test

8b4cd06

Add docs example + playground

6dc8363

Update snapshots

70f3e6f

changelog

7fe66b3

cee-chen force-pushed the callout branch from 2525a99 to 7fe66b3 Compare September 23, 2023 06:31

cee-chen reviewed Sep 23, 2023

View reviewed changes

src/components/call_out/call_out.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Sep 23, 2023

View reviewed changes

1Copenut reviewed Sep 25, 2023

View reviewed changes

[PR feedback] copy

0bc10f0

cee-chen approved these changes Sep 25, 2023

View reviewed changes

cee-chen enabled auto-merge (squash) September 25, 2023 17:41

This was referenced Sep 25, 2023

[EuiModal] Adjust the source order to announce EuiTitle before Close button #7222

Open

[EuiFlyout] Adjust the source order to announce EuiTitle before Close button #7223

Open

1Copenut approved these changes Sep 25, 2023

View reviewed changes

cee-chen merged commit 4a83e0e into elastic:main Sep 25, 2023
1 check passed

cee-chen mentioned this pull request Sep 25, 2023

Upgrade EUI to v88.5.0 elastic/kibana#166868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiCallOut] Add `onDismiss` prop #7156

[EuiCallOut] Add `onDismiss` prop #7156

kyrspl commented Sep 5, 2023 •

edited by cee-chen

Loading

ryankeairns commented Sep 5, 2023

kyrspl commented Sep 5, 2023

cee-chen Sep 23, 2023

1Copenut Sep 25, 2023

cee-chen Sep 25, 2023

1Copenut Sep 25, 2023

cee-chen commented Sep 23, 2023

1Copenut left a comment

1Copenut Sep 25, 2023

1Copenut left a comment

kibanamachine commented Sep 25, 2023

elasticmachine commented Sep 25, 2023

[EuiCallOut] Add onDismiss prop #7156

[EuiCallOut] Add onDismiss prop #7156

Conversation

kyrspl commented Sep 5, 2023 • edited by cee-chen Loading

Summary

QA

General checklist

ryankeairns commented Sep 5, 2023

kyrspl commented Sep 5, 2023

cee-chen Sep 23, 2023

Choose a reason for hiding this comment

1Copenut Sep 25, 2023

Choose a reason for hiding this comment

cee-chen Sep 25, 2023

Choose a reason for hiding this comment

1Copenut Sep 25, 2023

Choose a reason for hiding this comment

cee-chen commented Sep 23, 2023

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Sep 25, 2023

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 25, 2023

elasticmachine commented Sep 25, 2023

💚 Build Succeeded

History

[EuiCallOut] Add `onDismiss` prop #7156

[EuiCallOut] Add `onDismiss` prop #7156

kyrspl commented Sep 5, 2023 •

edited by cee-chen

Loading