[EuiIcon] Correct isMounted logic #6166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cee-chen
approved these changes
Aug 23, 2022
I'll do a final test build & drop it in CRA before merging 👍 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6166/ |
|
Pointed the failing CRA project at a recompiled EUI, icons now load. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6099
React.StrictMode, used by Create React App & others, does interesting things around component creation, mounting, and re-use in development mode to enforce lifecycle best practices. EuiIcon was setting its
isMountedproperty in the wrong place which led to no icons loading in some development environments (I was never able to reproduce the issue in EUI docs), as StrictMode would mount->unmount->mount components, causing EuiIcon to throw away the async icon when it loaded.I tested this by manually changing node_modules/@elastic/eui in a create-react-app, and confirmed the change (also applied manually) resolved this issue in an internal Elastic project.
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- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart