feat(banner): add support for new styles in penta #9891
Conversation
| export type BannerColor = 'red' | 'orangered' | 'orange' | 'gold' | 'green' | 'cyan' | 'blue' | 'purple'; | ||
|
|
||
| export type BannerStatus = 'success' | 'warning' | 'danger' | 'info' | 'custom'; |
There was a problem hiding this comment.
Exported union types so that consumers can import them if needed, I remember that being talked about at an office hours not long ago.
| interface StatusBanner extends BannerProps { | ||
| color?: never; | ||
| status?: BannerStatus; | ||
| } | ||
|
|
||
| export const Banner: React.FunctionComponent<BannerProps> = ({ | ||
| interface NonStatusBanner extends BannerProps { | ||
| color?: BannerColor; | ||
| status?: never; | ||
| } | ||
|
|
||
| export const Banner: React.FunctionComponent<StatusBanner | NonStatusBanner> = ({ |
There was a problem hiding this comment.
Tried this out as a way of preventing both color and status props at the type level, I'm not committed to it if we don't like it though.
There was a problem hiding this comment.
Not entirely sure how I feel about this. It's definitely an improvement from what was attempted in 91d7907 (with the main issue being it didn't render props in the prop table correctly, whereas this PR does).
At the same time I'm almost wondering if it'd be better to just keep the variant prop, and makes its type BannerStatus | BannerColor?
Otherwise this looks good. If anything we could have a followup discussion about it since it's only going into the v6 branch so I wouldn't block over this going in now.
|
Preview: https://patternfly-react-pr-9891.surge.sh A11y report: https://patternfly-react-pr-9891-a11y.surge.sh |
wise-king-sullyman
requested review from
a team,
andrew-ronaldson,
kmcfaul,
lboehling,
nicolethoen and
srambach
and removed request for
a team
| return styles.modifiers[color]; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
not a big deal and maybe more of a preference thing, but as color and status are used exactly the same way, i think i'd just get rid of getStatusOrColorModifier and replace the call with (color || status) && styles.modifiers[color || status]
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
LGTM. The palette colours may change in CSS a bit but that shouldn't impact the react work. Thanks @wise-king-sullyman
| interface StatusBanner extends BannerProps { | ||
| color?: never; | ||
| status?: BannerStatus; | ||
| } | ||
|
|
||
| export const Banner: React.FunctionComponent<BannerProps> = ({ | ||
| interface NonStatusBanner extends BannerProps { | ||
| color?: BannerColor; | ||
| status?: never; | ||
| } | ||
|
|
||
| export const Banner: React.FunctionComponent<StatusBanner | NonStatusBanner> = ({ |
There was a problem hiding this comment.
Not entirely sure how I feel about this. It's definitely an improvement from what was attempted in 91d7907 (with the main issue being it didn't render props in the prop table correctly, whereas this PR does).
At the same time I'm almost wondering if it'd be better to just keep the variant prop, and makes its type BannerStatus | BannerColor?
Otherwise this looks good. If anything we could have a followup discussion about it since it's only going into the v6 branch so I wouldn't block over this going in now.
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
Looks ok from the core side.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
8 participants
What: Closes #9856
Additional issues: