fix(NotificationBadge): add Button.variant to allowed NotificationBadgeVariants#12069
fix(NotificationBadge): add Button.variant to allowed NotificationBadgeVariants#12069logonoff wants to merge 1 commit intopatternfly:mainfrom
Button.variant to allowed NotificationBadgeVariants#12069Conversation
|
Preview: https://pf-react-pr-12069.surge.sh A11y report: https://pf-react-pr-12069-a11y.surge.sh |
There was a problem hiding this comment.
Change request below if this is something that is still approved by design. cc @andrew-ronaldson wdyt? Should NotificationBadge support plain styling (and if so, only allow plain or stateful as valid options?), or would this implementation be better suited to setting up a custom Button (after all all NotificationBadge is is a Button wwith only the necessary props exposed)?
| isExpanded?: boolean; | ||
| /** Determines the variant of the notification badge. */ | ||
| variant?: NotificationBadgeVariant | 'read' | 'unread' | 'attention'; | ||
| variant?: NotificationBadgeVariant | ButtonProps['variant'] | 'read' | 'unread' | 'attention'; |
There was a problem hiding this comment.
We would want to add a different prop, maybe buttonVariant, to pass into the Button variant prop. This prop is a bit confusingly named and should only be for passing to the Button state prop.
We would also need to only pass in anything to the Button state prop only if the button variant is stateful, otherwise the icon coloring on hover/focus gets messed up if the state modifier classes are applied with pf-m-plain.
|
cc @andrew-ronaldson For the design question above. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
4 participants
To be honest I don't know if these changes should be made to PF.. but
variant="plain"looks the most "in place" in the context of OCP's masthead and we should definitely at least have that variant supported in PFWhat: Closes #12066
Additional issues: openshift/console#15555