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

[EuiToast & Co.] Convert to function component, Emotion styling #6068

Merged
merged 15 commits into from
Jul 28, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 21, 2022

Summary

Converts EuiGlobalToastList to a function component. Pretty straightforward, but see line comments for a more involved explanation.
Converts EuiToast, EuiGlobalToastListItem and EuiGlobalToastList to styling with @emotion.

View by commit for the easiest review path!

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Comment on lines +243 to +252
// Toast dismissal side effect
// Ensure the callback has correct state by not enclosing it in `setTimeout`
useEffect(() => {
const toast = toastToDismiss;
// Because this is triggered by a setTimeout, and because React does not guarantee when
// state updates happen, it is possible to double-dismiss a toast
// including by double-clicking the "x" button on the toast
// so, first check to make sure we haven't already dismissed this toast
if (toast && toastIdToTimerMap.current.hasOwnProperty(toast.id)) {
dismissToastProp(toast);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only important change in the conversion to using hooks.
Previsouly props.dismissToast was called directly from within the setTimeout function and that was fine. With hooks, the closure inside useCallback would "lock" the state at time of instantiation and state would be outdated by the time the callback occurred.
This could be solved by requiring consumers to use the previous state param in their state management (setToastList((prevToasts) => prevToasts.filter(() => {})); but we can avoid the requirement with this side effect.

Comment on lines +51 to +53
${logicalCSS('padding-left', euiTheme.size.base)};
${logicalCSS('padding-right', euiTheme.size.base)};
${logicalCSS('padding-vertical', euiTheme.size.base)};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using -left and -right instead of -horizontal because these values are potentially overridden by :not(:empty) logic and -left/-right will not cancel -horizontal (different generated CSS properties).

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6068/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6068/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6068/

@thompsongl thompsongl marked this pull request as ready for review July 25, 2022 15:51
@elizabetdev elizabetdev self-requested a review July 26, 2022 16:01
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Looks good @thompsongl! 🎉

I tested in Chrome, Firefox and Safari. Works well! 🚀

I just have two suggestions. Let me know what you think.

src/components/toast/global_toast_list.styles.ts Outdated Show resolved Hide resolved
src/components/toast/toast.styles.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6068/

@thompsongl thompsongl merged commit cb0db20 into elastic:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants