Feat(Dropdown): add image & plain text support

[evwilkin]

What: Closes #3947

This PR adds:

• a new icon prop to the DropdownToggle component which adds support for including images in the dropdown toggle, before any component children.
• a new isPlainText prop added to the DropdownItem component which adds support for displaying non-interactive text as plain text within a dropdown item.
• renames the iconComponent prop within DropdownToggle to toggleIndicator to help differentiate it from new icon prop mentioned above; functionality remains unchanged.

Breaking changes

• iconComponent prop in DropdownToggle has been renamed to toggleIndicator, replace instances of iconComponent with toggleIndicator.

[patternfly-build]

PF4 preview: https://patternfly-react-pr-4038.surge.sh
A11y report: https://patternfly-react-pr-4038-coverage.surge.sh

[mattnolting]

Well done! LPTM

[redallen]

Clean!

[tlabaj]

The implementation looks fine, however this seem like a very specific use case for the dropdown. I took a look at the core issue and it seems like these changes were made to support combining the avatar and dropdown in the masthead. I would argue that Page Header is where the combining of the avatar and dropdown should happen from a React perspective. The consumer can put whatever they want in the toggle right now (i.e. an image and text) so to add a prop for an image just for PF specific styling related to the Masthead should be internal. For the consumer it does not make sense. I think we should discuss further how to do this. on the react side.

@evwilkin with our current implementation I would probably make an internal AvatarToggle (similar to the kebabToggle). This would be an internal Component to only be used by PageHeader. The Header would pass the AvatarToggle to dropdown.

cc @mcoker @mcarrano

[tlabaj]

I have some api concerns. I left a comment.

[mattnolting]

@tlabaj It was initially implemented for the avatar, but was abstracted to handle any image, thus .pf-c-dropdown__toggle-image rather than .pf-c-dropdown__toggle-avatar. As far as I can tell, there isn't an example that showcases adding an image to the dropdown button itself and it behaving as expected, though I could be wrong. Consider the following examples:

Without support

With support

[tlabaj]

@mnolting I tool a screenshot of how it look with just adding the icon to the toggle with current implementation. The dropdown below is the new implementation in this PR. From a react perspective, there is nothing preventing the consumer form adding an image to the toggle. Having a toggleImage prop for the Dropdown seems confusing to me given the fact that you can already place an image in the toggle.

[mattnolting]

@mcarrano @mceledonia wdyt?

[mcoker]

If I understand the question, I think this is the same feature as icons in the dropdown item. There is nothing stopping someone from placing an icon in a dropdown item, and it looks likes this:

However, we have a .pf-c-dropdown__menu-item-icon element that provides a container for the icon that handles spacing, alignment, etc. It allows us to manage the icon with relation to the other things in the toggle for the user. It looks like this:

The toggle icon element is basically the same thing. It allows us to manage the icon as part of the toggle layout, as opposed to a user just adding an icon and managing that layout themselves -- which they can still do if they want, just like with dropdown items icons.

[mcarrano]

@tlabaj While I agree that we don't currently have a use case for adding this feature outside of the page header, I'm OK if it is a general feature of the Dropdown. Seems inline with our modular component approach. But I'll support which ever way you want to go with this.

Maybe rather than toggleImage it should be 'toggleAvatar'?

[tlabaj]

@mcoker I think this is a little different. The .pf-c-dropdown__menu-item-icon wraps the whole item. Whereas pf-c-dropdown__toggle-image does not wrap the whole toggle, it wraps a portion of the toggle's children that has an image. The API seems confusing to me as it is in this PR right now.

@evwilkin My preferred recommendation without moving the css for the image out of the dropdown would be to create a new toggle like I said above. I think the use case is really specific here and there is to need to expose it. If we have demand for it later on, we can always export it.

@jschuler @dlabaj any thoughts here?

[jschuler]

What's the use-case for this @mcarrano ?

[jschuler]

@tlabaj I think it's probably not too uncommon to display icons for toggles and dropdown items and like exposing an option to easily add one if I need it. But it is a concern that the interface to achieve that differ here and in DropdownItem. I would suggest aligning them.
It would introduce more breaking changes, but I think they might be worthwhile just to have similar interfaces here.

This is what we currently have:

<DropdownToggle 
    id="toggle-id-9" 
    onToggle={this.onToggle} 
    iconComponent={CaretDownIcon}
    toggleImage={<UserIcon />}
>
	Ned Username
</DropdownToggle>

I think the iconComponent and toggleImage prop names are not the best here. I would recommend changing:

• iconComponent to toggleIcon (edit: or maybe just toggle?)
• toggleImage to itemIcon (edit: or maybe just icon?)

Then we can use the same prop name itemIcon for DropdownItem.

Currently:

<DropdownItem>
    <DropdownItemIcon>
      <CubesIcon />
    </DropdownItemIcon>
	Other action
</DropdownItem>

Make it:

<DropdownItem itemIcon={CubesIcon}>
  Other action
</DropdownItem>

[dlabaj]

@tlabaj I don't think that we should be exposing a second way of adding an image to the dropdown by introducing toggleImage that is available to consumers. Users can do this already without the need for this prop. It adds confusion to our user-facing API. If this is for an internal case then we should either create an internal component that has this image or not expose the API to external developers.

[jschuler]

@dlabaj Not sure consumers can do that themselves if we didn't render it for them, e.g. if you try

<DropdownToggle 
            id="toggle-id-9" 
            onToggle={this.onToggle} 
            iconComponent={CaretDownIcon}
          >
            <span className="pf-c-dropdown__toggle-image">
              <UserIcon />
            </span>Ned Username
          </DropdownToggle>

it won't render correctly because the entire child would be wrapped in pf-c-dropdown__toggle-text which is not correct. Also they would have to manually add the pf-c-dropdown__toggle-image class unless we made a component for that

[tlabaj]

@jschuler I don't think that is the intended API for dropdown icon.
I think it is this
<DropdownItem variant='icon'> <CubesIcon /> Other action </DropdownItem>

Thea variant changes class applied on the <a> or <button in the <li>

[tlabaj]

@jschuler @dlabaj I was saying that I think the class for the image belongs in the Masthead. If we want to keep it in the Dropdown, I was suggesting adding new toggle similar to kebabToggle, we would not have to export it to the consumers.

If we think that having an image in a toggle as the first child is a common use case, we can expose this to the consumer. The consumer can already add an icon to the toggle, it just does not have the PF styling that pf-c-dropdown__toggle-image provides.

[jschuler]

@tlabaj you want to wrap the icon with DropdownItemIcon since it applies a class that gives it proper padding. See example here https://www.patternfly.org/v4/documentation/react/components/dropdown#split-button-action

And yes, I accidentally dropped of variant='icon' in my example, which I believe vertically aligns the text to the icon. See, the API is too confusing :D

[tlabaj]

@jschuler I see that now. Looking at it now, I agree that the API for DropdownItem is not ideal. Specifically because I think that class pf-m-icon pf-c-dropdown__menu-item seems to expect the icon to be leading. Is that the case @mcoker? With the current implementation consumer may think they can put the icon anywhere in the menu item OR they may leave out the wrapper all together. But this is similar to other components like the Card where the user is responsible for wrapping something to get the proper styling.

@jschuler I agree that the API's should be consistent with the Toggle and the Item. Either we change DropdownItems or we create a new ToggelImage component similar to DropdownItemIcon?

[jschuler]

LGTM!

[tlabaj]

LGTM

[patternfly-build]

Your changes have been released in:

• @patternfly/react-catalog-view-extension@4.1.5
• @patternfly/react-core@4.6.2
• @patternfly/react-docs@5.1.14
• @patternfly/react-inline-edit-extension@4.2.5
• demo-app-ts@4.4.3
• @patternfly/react-integration@4.4.2
• @patternfly/react-table@4.2.2
• @patternfly/react-topology@4.1.5
• @patternfly/react-virtualized-extension@4.1.5

Thanks for your contribution! 🎉