Eui: Components that use the `iconType` property should also support an `icon` override

[haywoood]

I am using this wonderful library in a project which relies on the Google Closure Compiler for code compilation. Unfortunately, GCC does not support the dynamic import syntax used to enable iconType. There are other build tools that have trouble with this as well https://github.com/elastic/eui/issues/2467.

Since support for that syntax is not universal, and there is an escape hatch built into some components via the icon prop, I think it would be good to standardize on explicitly passing the icon component when developing 'internal' components.

An example would be updating the EuiNavDrawer component here to utilize the icon prop which EuiListGroupItem does support. I should note that the extraAction field is passed to a component that requires iconType and does not support an icon override. Those props end up here: https://github.com/elastic/eui/blob/master/src/components/button/button_icon/button_icon.tsx#L74.

Internally, I think it would be a good policy for any component that can accept iconType to also support icon, and EUI components that leverage icons should default to passing the icon component explicitly.

The GCC issue: https://github.com/google/closure-compiler/issues/2770

[snide]

This gets difficult fairly fast. While allowing icon to be passed into the iconType would work for surface level components, anything that uses icons in deeper components that don't expose the iconType props (essentially the entirety of forms or any high level component) wouldn't work. I certainly don't think we want to add that amount of props bloat to EUI to support this use case, and more likely would try to solve it in a more robust fashion, by having some optional package where icons were not dynamically imported.

Neither are on our current roadmap, and my guess is EUI will remain needing to work in a system that requires dynamic imports for some time.

[haywoood]

I think I follow what you're saying, but to be clear I'm advocating support for an icon property _in addition_ to iconType, that would be completely optional, basically copying how this component handles it https://github.com/elastic/eui/blob/master/src/components/list_group/list_group_item.tsx#L106.

Currently some components support both methods, and some don't, like https://github.com/elastic/eui/blob/master/src/components/button/button_icon/button_icon.tsx#L74.

Then during the development of components like NavDrawer, it would be good to default to the icon property versus iconType. This will make the components compatible with all bundlers without bloating the build, and will also give end users the ability to use iconType and EuiIcon if they wish.

It also doesn't have to be all or nothing. Perhaps I could submit a patch that updates NavDrawer to utilize the icon property, and update the button_icon component to support both iconType and icon.

[chandlerprall]

iconType is intended to be the more flexible, as it includes a ReactElement escape hatch - https://github.com/elastic/eui/blob/5348a47864042ad25f82a49d3e52b7da3b1e3446/src/components/icon/icon.tsx#L403

It does differ in that it is passed down to EuiIcon, but that is intentional as it receives some css styling, etc.

I'm curious, though, if the change is adding an icon escape hatch prop addresses the issue at all? NavDrawer still imports ListGroupItem still imports Icon, so compilers/bundlers would need to support the syntax anyway?

To that end, you're able to do this anywhere that IconType is used:

import { icon as EuiIconAppAddData } from '../../../../src/components/icon/assets/app_add_data.js';

...
  <EuiIcon type={EuiIconAppAddData} size="xl" />
...

Although functional, there are currently two issues with the above:

1. IconType is incorrectly defined as ReactElement instead of a JSXElementConstructor, this means TypeScript would prevent the above usage and EuiIcon's proptype throws a warning.
2. EuiIcon auto-applies some classes for the app* icons, it does this based on the icon prop's string value.

[haywoood]

Thanks for the thoughtful reply @chandlerprall I know you guys are never not slammed and I recognize this is not a high priority issue 🙂

I should clarify that the compiler doesn't die when encountering that import syntax, the code path just doesn't work. In the application we're working on the nested icons used in NavDrawer just don't show up, but everything is working other than the empty space.

To test I have a branch with the modest code changes I mentioned above and I was able to get the expand and collapse icons, as well as the nested lock/unlock icons showing by utilizing the icon property from EuiListGroupItem

<EuiListGroupItem
                buttonRef={node => (this.expandButtonRef = node)}
                label={this.state.isCollapsed ? sideNavExpand : sideNavCollapse}
                icon={
                  this.state.isCollapsed ? <MenuRightIcon /> : <MenuLeftIcon />
                }
                size="s"
                className={
                  this.state.isCollapsed
                    ? 'navDrawerExpandButton-isCollapsed'
                    : 'navDrawerExpandButton-isExpanded'
                }
                showToolTip={this.state.isCollapsed}
                extraAction={{
                  className: 'euiNavDrawer__expandButtonLockAction',
                  color: 'text',
                  onClick: this.sideNavLockClicked,
                  icon: this.state.isLocked ? (
                    <LockIcon className="euiIcon--small" />
                  ) : (
                    <LockOpenIcon className="euiIcon--small" />
                  ),
                  'aria-label': sideNavLockAriaLabel,
                  title: this.state.isLocked
                    ? sideNavLockExpanded
                    : sideNavLockCollapsed,
                  'aria-pressed': this.state.isLocked ? true : false,
                }}
                onClick={this.collapseButtonClick}
                data-test-subj={
                  this.state.isCollapsed
                    ? 'navDrawerExpandButton-isCollapsed'
                    : 'navDrawerExpandButton-isExpanded'
                }
              />

I'm also passing the actual component in the extraAction prop. To get that working I had to update the button_icon.tsx component to support props.icon (code) which it looks like it meant to since it has an if statement checking for iconType. This if branch is always called because iconType is a required property.

And you're right, I can totally import and use the icons directly and as @snide mentioned, the problem is with 'private' usages of iconType="icon-name" because in those situations I'm unable to modify how the icon is being used.

Our team will have to figure out some workaround as we can't change compilers, and we were thinking we could patch components as we run into issues (like that branch with NavDrawer).

[haywoood]

Thought I'd update this with how we're working around it. After we install EUI we replace the transpiled icon.js with a patched version that's checked into our project, relevant code change being the replacement of the async import call with a lookup using a function we provide on the window object:

    _defineProperty(_assertThisInitialized(_this), "loadIconComponent", function (iconType) {
      Promise.resolve().then(function () {
        return window.euiIconResolver(iconType);
      }).then(function (_ref) {

The provided resolver looks like this. So when we use a component that uses an icon we don't have, a message is printed to the console letting you know you need to add it here.

(ns app.utils.icon-hacks
  (:require
   [eui.icon-apps :as Apps]
   [eui.icon-cross :as Cross]
   [eui.icon-arrow-down :as ArrowDown]
   [eui.icon-arrow-up :as ArrowUp]
   [eui.icon-search :as Search]))

(def icon-map
  {"search"    Search
   "cross"     Cross
   "arrowDown" ArrowDown
   "arrowUp"   ArrowUp
   "apps"      Apps})

(defn eui-icon-resolve-fn [iconType]
  (if-let [resolved-icon (get icon-map iconType)]
    resolved-icon
    (do
      (js/console.warn (str "Failed to resolve icon: " iconType))
      (js/console.warn "Please import the icon into the deli.utils.icon-hacks namespace")
      (js/console.warn (str "eui.icon-" iconType)))))

And on application startup, we bind that function to the window so icon.js can use it.

This is obviously not great 😄 but we couldn't get anything else working. Hopefully the Google Closure Compiler supports async importing soon. If y'all want to close this feel free.

[haywoood]

Oh wow now I'm seeing this was recently added! https://github.com/elastic/eui/blob/684bbeed8d5960709f582c4c12ea90f7dffa5991/wiki/consuming.md#failing-icon-imports

I can clean this up a little bit now :)

[haywoood]

Thanks again for adding that function!