-
Notifications
You must be signed in to change notification settings - Fork 841
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
Add dismiss all toast button for EuiGlobalToastList component #7111
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
5da8e01
to
c595906
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
@@ -40,7 +40,14 @@ export interface Toast extends EuiToastProps { | |||
toastLifeTimeMs?: number; | |||
} | |||
|
|||
export interface EuiGlobalToastListProps extends CommonProps { | |||
interface EuiGlobalToastListClearAllProps<T> { | |||
supportClearAllToastsAction?: T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to have an explicit boolean
type here, or am I missing someting?
For the naming, something terser like clearAllEnabled
, allowClearAll
might be better. But naming is hard! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it'd be appropriate to add the boolean constraint, I've opted to move the prop definition into the EuiGlobalTotalListsProps
.
Thanks for the naming suggestion, I've opted for allowClearAll
it's succinct!
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
d1ddaf2
to
206786f
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
@eokoneyo this looks good. As for contrast, the text on background should be fine as EUI would ensure that 'out of the box'. Was the contrast concern in reference to the page background/underneath color? I'm not certain how real of an issue that is, but we could consider using a |
I think a button with fill would add too much visual noise, especially if we have this button's width match the width of the toasts. |
@andreadelrio the concern about having the button with primary color, was brought up here. TLDR; for cases where the "clear all" button shows up against a light gray background, e.g. the login page for Kibana, the color contrast might be an issue. We'd want the button to have a background that works across various backgrounds, I've also provided visuals for the button with the fill style for your consideration; |
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there's still some design discussion going on, so apologies for potentially jumping in a little too early with code review. Feel free to disregard these comments until design has settled if necessary.
Nice job on this Eyo, and thank you so much for the EUI contribution! ❤️
allowClearAll?: S; | ||
dismissAllToastsCallback?: [S] extends [true] ? () => void : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, the generic type you've added is way too fancy for EUI 😅 let's make it simpler, both new props can simply be optional, and we can make it clear via our props jsdocs that the callback does nothing without the flag:
allowClearAll?: S; | |
dismissAllToastsCallback?: [S] extends [true] ? () => void : never; | |
allowClearAll?: boolean; | |
/** | |
* Optional callback that fires when a user clicks the "Clear all" button. | |
* Ignored if `allowClearAll` is not `true`. | |
*/ | |
dismissAllToastsCallback?: () => void; |
Also, as Sébastien said, naming is indeed incredibly difficult! I'd like to propose new prop names based on similar functionalities in other components in EUI:
allowClearAll?: S; | |
dismissAllToastsCallback?: [S] extends [true] ? () => void : never; | |
/** | |
* Displays a "Clear all" button at the bottom of the toast list | |
* that allows users to dismiss all toasts in a single click. | |
*/ | |
showClearAllButton?: boolean; | |
/** | |
* Optional callback that fires when a user clicks the "Clear all" button. | |
* Ignored if `showClearAll` is not `true`. | |
*/ | |
onClearAllToasts?: () => void; |
The showX
naming is something we use very frequently across EUI and will be familiar to developers/consumers. For callbacks, the onX
naming is also something we generally stick to.
Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cee-chen I realize the generic is approach is a lot. I was hoping to make the expectation of the new props self-documenting, I'd switch to the jsdoc approach especially that it'd show up in the documentation too. Thanks for your feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally makes sense! And honestly the technique is really cool, I'll keep it my back pocket for the future! 🧑🎓
Do the name changes work for you by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too ... and yes the name changes do work, I'll be pushing changes that incorporate them
@andreadelrio I would like to opt for minimal amount of configuration that relates to configuring the dismiss all toast functionality , especially given the fact that we are applying these configuration through the
@andreadelrio thanks for pointing this out, I'm aware of this particular issue. I didn't look into this further, because I wanted to get feedback early if this the direction to go before polishing up the PR. |
@andreadelrio I fixed the issue with the button height issue, although whilst I was resolving the issue with the button I encountered an edge case where there's a lot of toast, the GIF below shows the scroll behaviour of the clear all button when there's a lot of toasts I experimented with having the button sticky so the clear button stays visible but I'm not sold on the UX of this approach I'd like your opinion on the appropriate pattern to adopt. |
@eokoneyo thanks for the fix and exploring the sticky button option. I personally think we should make it sticky, this button should be easily accessible at all times, especially when there's a storm of toasts. |
7bb47eb
to
a7501d8
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
a7501d8
to
3fe0d93
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
/** | ||
* Displays a "Clear all" button at the bottom of the toast list | ||
* that allows users to dismiss all toasts in a single click. | ||
*/ | ||
showClearAllButton?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach we could take with this prop that would tie in nicely to the above question about a customizable threshold:
/** | |
* Displays a "Clear all" button at the bottom of the toast list | |
* that allows users to dismiss all toasts in a single click. | |
*/ | |
showClearAllButton?: boolean; | |
/** | |
* At this threshold, a "Clear all" button will display at the bottom of the toast list | |
* that allows users to dismiss all toasts in a single click. | |
* | |
* Defaults to `3`. Set to `0` to disable the button entirely. | |
*/ | |
showClearAllButtonAt?: number; |
and then later down below in the props destructuring/defaults, we'd do:
showClearAllButtonAt = 3,
Thoughts? Worth doing or nah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cee-chen I like this approach especially that it consolidates 2 concerns, but I'd also like to point out that by adopting this pattern and setting the default to 3 we'd be making the "dismiss button" a first class functionality of the toast list, whilst this is fine for us (Kibana) it'd be a breaking change of some sort of other eui consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That been said, perhaps we set the default to 0
and have every consumer set their own threshold, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'd be in favor of adopting this new pattern as a default. I think it's a very sensible one that I wish other apps had (I use an online productivity app called Habitica that generates 3 toasts per action, and I can sometimes sit there clicking/dismissing individual toasts for a minute straight 🥲).
Other EUI consumers can simply disable the new feature in less than one line if they don't want it, so I think it's fairly safe to add. @andreadelrio, any opinions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of making it the default, seems like a nice UX consideration that's easy to disable if needed.
3fe0d93
to
a6ffc8f
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
c54a3ca
to
2d185ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments/change requests around test architecture and dev readability - I know this is a lot, apologies and please do feel free to ping me if anything's unclear and it'd be more helpful to pair on this live!
import { mount } from 'enzyme'; | ||
import { requiredProps, findTestSubject } from '../../test'; | ||
import { render } from '../../test/rtl'; | ||
import { render, findByTestSubject, queryByTestSubject } from '../../test/rtl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These RTL APIs can be destructured directly from render()
as opposed to being top level exports, which I believe is a general RTL best practice. See below comments for examples
import { render, findByTestSubject, queryByTestSubject } from '../../test/rtl'; | |
import { render } from '../../test/rtl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know the recommended RTL best practice for querying is to use screen, I see now that we expose a custom screen util that has our custom matchers registered on it so I've opted to use it instead, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, you're totally right - TIL that RTL recommends screen for querying. Huh. Interesting!
To be totally honest with you, I don't always agree with Kent C Dodds (although he's obviously a super smart person) and some of the assertions he makes are very opinionated within very limited vs general contexts 😅
Here's the reasons why I'd prefer to continue destructuring our queries from render()
as opposed to importing them:
- The rest of the EUI codebase and existing RTL tests do this. This is honestly probably the biggest reason; code consistency trumps almost all other factors.
- There's very little impact on actual behavior (unless you need to query something outside the returned component container, e.g. portals), so basically we're just discussing syntactical sugar here. Just in my personal opinion, for 90% of use cases, using the returned queries is nicer because:
- It's fewer characters to type - e.g.
getByTestSubject('foo')
vs.screen.getByTestSubject('foo')
vs.getByTestSubject(container, 'foo')
- It's encapsulated to the test only (so one test could use
getX
and the other could usequeryY
and it's very clear which test is using what)
- It's fewer characters to type - e.g.
For the purposes of this PR, I'd prefer to stick with the API/syntax we've been using across EUI for consistency, as mentioned, but I can certainly bring it to the rest of the EUI team after this to ask them whether they'd prefer to switch to screen
to follow RTL's recommendations. But that's a change we would want to make across the codebase / all existing RTL tests to maintain consistency, and would be outside the scope of the current PR.
Hope that all makes sense - please let me know if not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Cee, thanks for outlining the rationale to opt for using destructured matchers. Considering that the codebase is currently in the process of migrating to RTL deciding wether to adopt screen or not is definitely a conversation worth having, it's use case is valid. In the term I'll use the matchers directly, I do agree consistency trumps other factors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to close the loop on this - the EUI team discussed this briefly in our sync today and agreed we generally prefer the render()
destructured syntax. We're updating our wiki docs (#7199) to note that this is a conscious deviation from RTL's recommendations.
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
cbe39a3
to
7ab35a3
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience with the PR review process and particularly with my back and forth on tests! Again, really great work on this feature Eyo, it was a pleasure working with you!
@andreadelrio Do you mind updating the Figma library with this new button? There's an "Examples" section with a list of 4 toasts that I'd love to see the new clear all button show at the bottom of |
Preview documentation changes for this PR: https://eui.elastic.co/pr_7111/ |
💚 Build Succeeded
History
|
EUI `88.2.0` ➡️ `88.3.0` ## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0) - `EuiGlobalToastList` now shows a "Clear all" button by default once above a certain number of toasts (defaults to 3). This threshold is configurable with the `showClearAllButtonAt` prop ([#7111](elastic/eui#7111)) - Added an optional `onClearAllToasts` callback to `EuiGlobalToastList` ([#7111](elastic/eui#7111)) - Added the `value`, `onChange`, and `onCancel` props that allow `EuiInlineEdit` to be used as a controlled component ([#7157](elastic/eui#7157)) - Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`, `transitionTopIn`, and `transitionTopOut` icon glyphs. ([#7168](elastic/eui#7168)) **Bug fixes** - Fixed `EuiInlineEdit` components to correctly spread `...rest` attributes to the parent wrapper ([#7157](elastic/eui#7157)) - Fixed `EuiListGroupItem` to correctly render the `extraAction` button when `showToolTip` is also passed ([#7159](elastic/eui#7159)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.3.0 ([#7125](elastic/eui#7125)) - Updated `@types/lodash` to v4.14.198 ([#7126](elastic/eui#7126)) **Accessibility** - `EuiAccordion` now correctly respects reduced motion settings ([#7161](elastic/eui#7161)) - `EuiAccordion` now shows a focus outline to keyboard users around its revealed children on open ([#7161](elastic/eui#7161)) **CSS-in-JS conversions** - Converted `EuiSplitPanel` to Emotion ([#7172](elastic/eui#7172))⚠️ As a quick heads up, serverless tests appear to have been extremely flake/failure-prone the last couple weeks, particularly Cypress tests. We've evaluated the listed failures and fixed ones that were related to changes in this PR, and we're relatively confident the remaining failures are not related to changes from EUI. Please let us know if you think this is not the case. --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Jon <jon@elastic.co>
⚠️ NOTE: This PR is a copy of #166292 (which was reverted due to failing Storybook builds). This is the same exact PR but with Storybook building fixed. --- EUI `88.2.0` ➡️ `88.3.0` ## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0) - `EuiGlobalToastList` now shows a "Clear all" button by default once above a certain number of toasts (defaults to 3). This threshold is configurable with the `showClearAllButtonAt` prop ([#7111](elastic/eui#7111)) - Added an optional `onClearAllToasts` callback to `EuiGlobalToastList` ([#7111](elastic/eui#7111)) - Added the `value`, `onChange`, and `onCancel` props that allow `EuiInlineEdit` to be used as a controlled component ([#7157](elastic/eui#7157)) - Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`, `transitionTopIn`, and `transitionTopOut` icon glyphs. ([#7168](elastic/eui#7168)) **Bug fixes** - Fixed `EuiInlineEdit` components to correctly spread `...rest` attributes to the parent wrapper ([#7157](elastic/eui#7157)) - Fixed `EuiListGroupItem` to correctly render the `extraAction` button when `showToolTip` is also passed ([#7159](elastic/eui#7159)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.3.0 ([#7125](elastic/eui#7125)) - Updated `@types/lodash` to v4.14.198 ([#7126](elastic/eui#7126)) **Accessibility** - `EuiAccordion` now correctly respects reduced motion settings ([#7161](elastic/eui#7161)) - `EuiAccordion` now shows a focus outline to keyboard users around its revealed children on open ([#7161](elastic/eui#7161)) **CSS-in-JS conversions** - Converted `EuiSplitPanel` to Emotion ([#7172](elastic/eui#7172)) --------- Co-authored-by: Bree Hall <briannajdhall@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jon <jon@elastic.co>
Summary
Takes a shot at resolving the "Clear All Button" requirement of #6945, referenced by elastic/kibana#162646
Approach
EuiGlobalToastListItem
component to keep the clear all button fixed at the bottom of all toasts, having the clear all button on the top pushes the button out of the screen once the list of visible toast exceeds the current screen size.showClearAllButtonAt
with a default of 3, that denotes when the threshold for displaying the dismiss all toasts button, passing a value of0
would opt one out of displaying the dismiss all toasts button. An accompanying proponClearAllToasts
is introduced to register a method that will be invoked if provided when the clear all button is trigged.Usage
Caveat;
This approach requires that the dismissToast function passed to the
EuiGlobalToastListItem
, if it performs state updates adopts the updater function style to ensure the state updates are accurately computed (see here for an explanation), especially because we iterate over the existing toasts and remove them.Visuals
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@default
if default values are missing) and playground toggles- [ ] Checked Code Sandbox works for any docs examples