Skip to content

[CSS-in-JS] Convert EuiErrorBoundary#6053

Merged
thompsongl merged 6 commits intoelastic:mainfrom
thompsongl:css-in-js/euierrorboundary
Jul 20, 2022
Merged

[CSS-in-JS] Convert EuiErrorBoundary#6053
thompsongl merged 6 commits intoelastic:mainfrom
thompsongl:css-in-js/euierrorboundary

Conversation

@thompsongl
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl commented Jul 14, 2022 •
edited
Loading

Summary

Converts EuiErrorBoundary to @emotion styling.
Note that if you run this locally, the error overlay on the Error Boundary docs page is expected.

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
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@thompsongl thompsongl mentioned this pull request Jul 14, 2022
Closed
49 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 14, 2022

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

@thompsongl thompsongl marked this pull request as ready for review July 14, 2022 18:53
elizabetdev
elizabetdev approved these changes Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
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.

LGTM! 🎉

I just have a few questions and suggestions. Tested in Chrome, Safari, Edge, and Firefox.

src/components/error_boundary/error_boundary.tsx
EuiErrorBoundaryProps,
type EuiErrorBoundaryExtendedProps = EuiErrorBoundaryProps & WithEuiThemeProps;

export class _EuiErrorBoundary extends Component<
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev Jul 18, 2022

Choose a reason for hiding this comment

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

Should this component be converted to a function component?

Copy link
Copy Markdown
Contributor Author

@thompsongl thompsongl Jul 18, 2022

Choose a reason for hiding this comment

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

Unfortunately it's not yet possible: https://reactjs.org/docs/hooks-faq.html#do-hooks-cover-all-use-cases-for-classes

We need componentDidCatch for this to function and there is no way to achieve that without a class component.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 18, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 19, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 19, 2022

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

@elizabetdev
Copy link
Copy Markdown
Contributor

elizabetdev commented Jul 20, 2022

@thompsongl it's looking good in Safari 15.4! 🎉

Screenshot 2022-07-20 at 13 51 49

@thompsongl thompsongl merged commit 74edbd0 into elastic:main Jul 20, 2022
@cee-chen cee-chen mentioned this pull request Jul 27, 2022
Closed
@cee-chen cee-chen mentioned this pull request Aug 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@elizabetdev elizabetdev elizabetdev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

emotion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thompsongl @kibanamachine @elizabetdev