INavLink iconClassName property is not reflected in the UI

[manasark]

Bug Report

• Package version(s): (5.64.1)
• Browser and OS versions: (All browsers and Windows 10)

Priorities and help requested (not applicable if asking question):

Requested priority: ( High )

Products/sites affected: (IRIS Studio)

Describe the issue:

We are using an INavLink component which has an iconClassName (Classname to apply to the icon link) property to render a style icon on the UI. However, we have noticed that the style icon is not rendered.

We have recently upgraded to the package version 5.64.1 and noticed this issue.

Actual behavior:

Style icon is not rendered on the UI.

Expected behavior:

This should render the style icon on the UI.

[KevinTCoughlin]

Hey @manasark, you're right. It looks like we used to add iconClassName to the<Icon/> component, but since then we've refactored the nav links with icons to extend from <ActionButton/> and haven't carried over the class name property. Investigating a fix / workaround now.

https://github.com/OfficeDev/office-ui-fabric-react/blob/5.0/packages/office-ui-fabric-react/src/components/Nav/Nav.tsx#L152

[KevinTCoughlin]

@manasark would applying the class name to the icon link <a ...> be sufficient for you to target the inner icon <i>? I have a patch that would do that, but then I wonder if the property name is misleading as it seems to imply its targeting the icon directly. It looks like that's what it used to do. Currently, it doesn't seem like there's another way to apply a custom class to that <ActionButton /> within a <NavLink />, here's the LoC:

https://github.com/OfficeDev/office-ui-fabric-react/blob/3c5e9c98329dbd3df53024ad19acfcfd96993303/packages/office-ui-fabric-react/src/components/Nav/Nav.base.tsx#L145

I'll open the change for the team to weigh-in.

[KevinTCoughlin]

The least invasive change would be this, but again I think the property name may be misleading:

master...KevinTCoughlin:keco/nav-iconClassName

Seems to have been broken for a bit. Thanks for the report 👍.

[jordandrako]

@oengusmacinog for visibility. This may be related to the recent mergeStyles conversion and we're investigating a refactor anyway

[oengusmacinog-zz]

@jordandrako it is not. Doesn't look like it was ever implemented, or perhaps was sacrificed in the refactor mentioned above

[KevinTCoughlin]

Ya, it looks like it got lost in a refactor from what I could tell. @oengusmacinog any opinion on a solution here? I have a pretty good idea of how to get a class name onto either the <a/> or the inner <i/>, but if we went with anchor the prop should probably be renamed.

[oengusmacinog-zz]

I think the best way to do this would be to add the className prop to IIconProps, and implement it with merge styles - this component is currently set up with Icon.classNames.ts - and then you could pass the classname to the link icon like so:

<Nav
  groups={
    [
      {
        links:
        [
          {
            name: 'Delete',
            url: 'http://cnn.com',
            onClick: this._onClickHandler2,
            iconProps: {
              iconName: 'Delete',
              className: 'myClass'
            },
            key: 'key9'
          }
        ]
      }
    ]
  }
/>

[KevinTCoughlin]

Thanks for the pointer @oengusmacinog, it does look like @manasark can do what they need to do without any library change. Here's a Codepen demonstrating the above: https://codepen.io/kevintcoughlin/pen/aGoGvm?editors=1010. It applies the class custom-foo to the button's <i> icon for "Delete".

Perhaps we should remove iconClassName from Nav.types.ts since its unused. Not sure if its worth marking as deprecated for some time.

[manasark]

@KevinTCoughlin Does this mean that we should only use iconProps for the style icon? Can you please implement the same functionality for iconClassName too so that it is backward compatible and doesn't require any code change from us?

[manasark]

@KevinTCoughlin @dzearing Any update on the above request?