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.

Sign up for GitHub

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

Jump to bottom

[EuiIcon]: Add appendIconComponentCache function #3481

Merged
merged 5 commits into from
May 26, 2020
Merged

[EuiIcon]: Add appendIconComponentCache function #3481

merged 5 commits into from
May 26, 2020

Conversation

dannya
Copy link
Contributor

@dannya dannya commented May 14, 2020 •
edited
Loading

@chandlerprall this is a small extension to your recent caching changes to EuiIcon in #3404

  • Add appendIconComponentCache function to complement the recently-added clearIconComponentCache function.

Use case is to work around issues with the dynamic loading: if an icon is not in the cache, an attempt is made to fetch it dynamically, however some environments (such as using certain bundlers, like Parcel: parcel-bundler/parcel#112) do not transpile this functionality correctly.

Being able to manually manipulate the icon cache allows a work around for this issue.

Summary

Provide a detailed summary of your PR. Explain how you arrived at your solution. If it includes changes to UI elements include a screenshot or gif.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest 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

@dannya
[EuiIcon]:
0142c62 
* Add appendIconComponentCache function to complement the recently-added clearIconComponentCache function.

Use case is to work around issues with the dynamic loading: if an icon is not in the cache, an attempt is made to fetch it dynamically, however some environments (such as using certain bundlers, like Parcel: parcel-bundler/parcel#112) do not transpile this functionality correctly.

Being able to manually manipulate the icon cache allows a work around for this issue.
@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented May 14, 2020 •
edited
Loading

💚 CLA has been signed

@chandlerprall
Copy link
Contributor

chandlerprall commented May 14, 2020 •
edited
Loading

I like this idea! Have you tested this change with parcel?

(Also, as our cla check service mentioned, please sign our Contributor Agreement using the email address associated with your github account. We can't accept a pull request without this)

@dannya
Copy link
Contributor Author

dannya commented May 14, 2020 •
edited
Loading

@chandlerprall I've tested this with Parcel, solves the issue I was facing in my own project. Here is some sample usage (preloading icons used in the EuiInMemoryTable component):

import { appendIconComponentCache } from '@elastic/eui/es/components/icon/icon';

import { icon as EuiIconArrowDown } from '@elastic/eui/es/components/icon/assets/arrow_down';
import { icon as EuiIconArrowLeft } from '@elastic/eui/es/components/icon/assets/arrow_left';

appendIconComponentCache(
  'arrowDown', EuiIconArrowDown
);
appendIconComponentCache(
  'arrowLeft', EuiIconArrowLeft
);

Once I saw the @cla-checker-service message above, I signed the CLA (not sure if the process is automatic or if it needs to be manually processed).

Thanks,
Danny

@dannya
Copy link
Contributor Author

dannya commented May 14, 2020

Although it doesn't directly solve the issue, this addition also might assist with cases like #2467

@chandlerprall
Copy link
Contributor

I threw a PR your way to clean this up a bit - wonderscore#1

Went with the PR route instead of requesting changes as the tests & documentation itself didn't feel like straight-forward asks.

One thought I had, given your example, is if the function should instead take an object of icon keys : components. Seems like it would clean up the usage a bit?

@dannya dannya marked this pull request as draft May 20, 2020 00:25
@dannya
[EuiIcon]: Implement suggested change to new appendIconComponentCache…
cffcab0 
… function: One or more icon(s) are passed in as an object of iconKey (string): IconComponent
@dannya dannya marked this pull request as ready for review May 20, 2020 23:03
@dannya
Copy link
Contributor Author

dannya commented May 20, 2020

I threw a PR your way to clean this up a bit - wonderscore#1

Went with the PR route instead of requesting changes as the tests & documentation itself didn't feel like straight-forward asks.

Merged back into this PR.

One thought I had, given your example, is if the function should instead take an object of icon keys : components. Seems like it would clean up the usage a bit?

Good idea, implemented in this PR.

@chandlerprall
Copy link
Contributor

jenkins test this

chandlerprall
chandlerprall suggested changes May 21, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes look good! Let's add two things and this should be good to merge in

  • needs an entry at the top of CHANGELOG.md mentioning the new function and referencing this PR
  • appendIconComponentCache should get a mock entry in icon.testenv.tsx - this file is provided for applications using EUI (such as Kibana) to automatically mock functionality those apps shouldn't care about - like all this dynamic loading - but it needs to have the same API as the source file.

@kibanamachine
Copy link

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

@dannya
[EuiIcon]:
b43f9b3 
* Define an empty no-op mock of the `appendIconComponentCache` function in icon.testenv.tsx
* Update the main CHANGELOG.md with a summary of the new `appendIconComponentCache` EuiIcon feature.
@dannya dannya requested a review from chandlerprall May 26, 2020 00:17
@chandlerprall chandlerprall mentioned this pull request May 26, 2020
Merged
@chandlerprall
Copy link
Contributor

jenkins test this

chandlerprall
chandlerprall approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Will merge on green CI.

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

known flaky unit test

jenkins test this

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

Different flaky reason.

jenkins test this

@kibanamachine
Copy link

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

@chandlerprall chandlerprall merged commit 684bbee into elastic:master May 26, 2020
@thompsongl thompsongl mentioned this pull request Jun 3, 2020
Merged
@dannya dannya deleted the feature/icon-cache-append branch June 22, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dannya @kibanamachine @chandlerprall