salesforce / design-system-react

Salesforce Lightning Design System for React
https://design-system-react-site.herokuapp.com/
BSD 3-Clause "New" or "Revised" License
921 stars 419 forks source link

Vertical Navigation: Missing icon and notification #2934

Closed gnzzz closed 3 years ago

gnzzz commented 3 years ago

Currently the Vertical Navigation is missing

from the SLDS design.

I'm proposing adding two optional fields, similar to how Badge is done, in the Item object to define an Icon and a Badge for notifications. For example:

[
  {
    id: 'store',
    label: 'Store',
    items: [
      {
        id: 'credit_store',
        label: 'credit',
        url: 'store/credit',
        icon: <Icon category="utility" name="moneybag" size="xx-small" />,
        notification: <Badge id="badge-base-example" content="423 Credits Available" />,
      },
      { id: 'transactions_store', label: 'Transactions', url: 'store/transactions' },
    ],
  }
];
welcome[bot] commented 3 years ago

Thanks for opening your first issue! :wave: If you have found this library helpful, please star it. A maintainer will try to respond within 7 days. If you haven’t heard anything by then, please bump this thread.

gnzzz commented 3 years ago

I realised there is some inconsistency in the way that icons are done for other components when I started to implement this.

For badges the prop is a component:

<Badge
    id="badge-base-example-light"
    color="light"
    content="423 Credits Available"
    icon={<Icon category="utility" name="moneybag" size="xx-small" />}
/>

and for buttons it's passed as the props of the icon with icon as a prefix:

<Button
    assistiveText={{ icon: 'Icon Bare Small' }}
    iconCategory="utility"
    iconName="settings"
    iconSize="small"
    iconVariant="bare"
    onClick={() => {
        console.log('Icon Bare Clicked');
    }}
    variant="icon"
/>

@interactivellama which of these approaches is the current preferred way?

interactivellama commented 3 years ago

Good question! In short, the Badge way is preferred since it limits "duplicate" props.

See https://github.com/salesforce/design-system-react/blob/master/docs/codebase-overview.md#reuse-existing-component-props-by-using-component-no-button-iconclassname-

interactivellama commented 3 years ago

Also, thanks for wanting to contribute. I like the proposal. What do you think about notificationBadge instead of notification so that consumers already know what to pass in?

Other than that, props proposal approved. Please move forward with implementation when convenient.

gnzzz commented 3 years ago

Thanks for the feedback. Yes, notificationBadge seems to make more sense to make it clear what it does.

gnzzz commented 3 years ago

I've opened an MR for this change. I struggled to get the typings correct and ended up using any which isn't ideal. If you have some suggestion to get that right then I'll change it.

gnzzz commented 3 years ago

@interactivellama is there any further changes needed on the PR?

interactivellama commented 3 years ago

@gnzzz Thanks for the ping! PR is merged now. We should have a version out in the next few days.