Flexible alert actions #3822
Flexible alert actions #3822
Conversation
seanforyou23
added
Breaking change 💥
|
PF3 preview: https://patternfly-react-pr-3822-pf3.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #3822 +/- ##
==========================================
+ Coverage 71% 71.02% +0.02%
==========================================
Files 785 785
Lines 10638 10639 +1
Branches 2314 2313 -1
==========================================
+ Hits 7553 7556 +3
+ Misses 2655 2653 -2
Partials 430 430
Continue to review full report at Codecov.
|
karelhala
left a comment
karelhala
left a comment
There was a problem hiding this comment.
Ooooh nice! This looks really nice!
seanforyou23
force-pushed
the
alert-action-enhancement
branch
from
29a3707 to
b291f09
Compare
|
needs to be rebased |
seanforyou23
force-pushed
the
alert-action-enhancement
branch
2 times, most recently
from
9972df6 to
cff4c7d
Compare
seanforyou23
force-pushed
the
alert-action-enhancement
branch
from
cff4c7d to
b1e5043
Compare
…r alertactionlink
seanforyou23
force-pushed
the
alert-action-enhancement
branch
from
b1e5043 to
41ed8d4
Compare
| /** Action button to put in the Alert. Should be <AlertActionLink> or <AlertActionCloseButton> */ | ||
| action?: React.ReactNode; | ||
| /** Action button to put in the Alert. Often <AlertActionLink> or <AlertActionCloseButton> */ | ||
| action?: AlertActionRenderer | React.ReactNode; |
There was a problem hiding this comment.
Isn't this really React.FunctionComponent<{ title: React.ReactNode, variantLabel: string }>?
| <h4 className={css(styles.alertTitle)}>{getHeadingContent}</h4> | ||
| {children && <div className={css(styles.alertDescription)}>{children}</div>} | ||
| <AlertContext.Provider value={{ title, variantLabel }}> | ||
| {action && (typeof action === 'object' || typeof action === 'string') && ( |
There was a problem hiding this comment.
Perhaps a better API would be to leave action's type as React.ReactNode and use React.cloneElement to add the title and variantLabel props? This way class components could be passed as well, and we don't have to amend the API.
{React.isValidElement(action) && (
<div className={css(styles.alertAction)}>
{React.cloneElement(action as React.ReactElement, {
title,
variantLabel
})}
</div>
)}Consumers would then be able to use added title and variantLabel props like this:
<Alert action={({title, variantLabel}) => <div>{title}{variantLabel}</div>} />OR like this:
class MyAlertAction extends React.Component {
render() {
return <div>{this.props.title}{this.props.variantLabel}</div>;
}
}
<Alert action={<MyAction />} />
| export interface AlertActionLinkProps extends ButtonProps { | ||
| /** Content rendered inside the AlertLinkAction */ | ||
| children?: string; | ||
| children: string; |
There was a problem hiding this comment.
Should be ReactNode so it could work with i18n libraries for example
| <div className={css(styles.alertAction)}>{action}</div> | ||
| )} | ||
| {action && typeof action === 'function' && ( | ||
| <div className={css(styles.alertAction)}>{action({ title, variantLabel })}</div> |
There was a problem hiding this comment.
I don't understand, couldn't this be
<div className={css(styles.alertAction)}>{action}</div> and since consumer is passing in title and variantLabel as props they already have knowledge of the props so aren't we just giving it back to them?
8 participants
What: Splitting out some work from #3771 - allowing actions to be any custom content, not just our action/close buttons.
Additional issues: #3771
Breaking changes
actionprop uses new type ofAlertActionRenderer | React.ReactNode. Previous valid values remain valid, but the type applied may need to be updated accordingly.