-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-3685, LG-2632: adds backButtonProps and cancelButtonProps to FormFooter component #2226
Conversation
🦋 Changeset detectedLatest commit: 8c15d10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +264 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
c778713
to
cc86ed2
Compare
b1cce85
to
cc257ec
Compare
interface BaseButtonProps { | ||
text?: string; | ||
onClick?: React.MouseEventHandler<HTMLButtonElement>; | ||
} | ||
|
||
interface BackButtonProps extends BaseButtonProps { | ||
leftGlyph?: React.ReactElement | null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow users to fully customize the buttons? The way I read the ticket, the intent is not only to customize the click handler & left glyph, but allow for further customization.
@bruugey I left a comment in the JIRA ticket, since I'm a bit confused about the AC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded in JIRA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think if we're going as far as accepting a prop object, I would prefer a slot API instead. That's the route we took for CanvasHeader, and I'm sure other devs would appreciate the customizability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the diff here is that we don't want consumers to change the button variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah from my discussion with design, they only want to introduce button icon customization
so then, customization should be restricted to text + click handler + icon
interface BaseButtonProps { | ||
text?: string; | ||
onClick?: React.MouseEventHandler<HTMLButtonElement>; | ||
} | ||
|
||
interface BackButtonProps extends BaseButtonProps { | ||
leftGlyph?: React.ReactElement | null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think if we're going as far as accepting a prop object, I would prefer a slot API instead. That's the route we took for CanvasHeader, and I'm sure other devs would appreciate the customizability
… maintain backwards compatibility
cc257ec
to
557c7e6
Compare
96ac96b
to
f484b83
Compare
|
||
import { PrimaryButtonProps } from './PrimaryButton'; | ||
|
||
type CustomButtonProps = Pick< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
✍️ Proposed changes
cancelButtonProps
andbackButtonProps
toFormFooter
component for customizing buttons.cancelButtonText
,onCancel
,backButtonText
, andonBackClick
props as deprecated.🎟 Jira ticket: LG-3685 | LG-2632
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changes🧪 How to test changes
Can see the new prop usage in storybook and added unit test blocks