LG-2719 | LG-3260: refactor Button to use InferredPolymorphic instead of Box #2695
+230
−225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 2eb02b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
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: +26 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
shaneeza
reviewed
Feb 6, 2025
shaneeza
reviewed
Feb 6, 2025
| forwardRef, | ||
| ) { | ||
| const { darkMode } = useDarkMode(darkModeProp); | ||
| export const Button = InferredPolymorphic<ButtonProps, 'button'>( |
There was a problem hiding this comment.
We should export a prop type that is also wrapped in InferredPolymorphic. Similar to splitButton. This way, if consumers need to extend the type, it will include the inferred polymorphic props.
shaneeza
reviewed
Feb 6, 2025
| onClick: isInteractive | ||
| ? onClick | ||
| : (e: React.MouseEvent) => e.preventDefault(), | ||
| href: isInteractive ? rest.href : undefined, |
There was a problem hiding this comment.
I think this href is being overwritten by the ...rest below it. Do we need this line?
There was a problem hiding this comment.
huh.. that wasn't changed in this PR 😅 I'll take a look and see if we can axe it
There was a problem hiding this comment.
I can't destructure href from the rest object to allow passing it to useInferredPolymorphic, but I moved this below the ...rest, so it should actually be working now
shaneeza
reviewed
Feb 6, 2025
| ({ children, ...props }: ButtonProps, ref) => ( | ||
| <Button {...props} ref={ref}> | ||
| <Button {...props} as="button" ref={ref}> |
There was a problem hiding this comment.
curious: why do we need to pass the as prop if we are passing button here: const { Component } = useInferredPolymorphic(as, rest, 'button');
There was a problem hiding this comment.
Paired with Adam on this, and it should be correct now without needing to specify the as prop
There was a problem hiding this comment.
It was a typescript issue. It wasn't able to calculate what the element would be which would cause the ref to have a type error
stephl3
changed the title
stephl3
commented
Feb 6, 2025
Comment on lines
+14
to
+16
| type CustomConfirmButtonProps = Omit<BaseButtonProps, 'variant' | 'onClick'> & | ||
| CustomButtonOnClick; | ||
| type CustomCancelButtonProps = Omit<ButtonProps, 'onClick' | 'children'> & | ||
| type CustomCancelButtonProps = Omit<BaseButtonProps, 'onClick' | 'children'> & |
There was a problem hiding this comment.
Confirmed that primary consumers' ConfirmationModal instances don't currently specify as or href as keys in these object props: https://github.com/search?q=(org:10gen+OR+org:evergreen-ci+OR+org:mongodb+OR+org:mongodb-js+OR+org:mongodb-labs+OR+org:wiredtiger)+AND+%22%3CConfirmationModal%22+AND+(confirmButtonProps%3D+OR+cancelButtonProps%3D)+AND+(as:+OR+href:)&type=code
| @@ -35,7 +35,7 @@ export type InlineMessageFeedbackProps = Required< | |||
| /** | |||
| * Override props for the submit Button | |||
| */ | |||
| submitButtonProps?: ButtonProps; | |||
| submitButtonProps?: BaseButtonProps; | |||
There was a problem hiding this comment.
Confirmed that primary consumers' InlineMessageFeedback instances don't currently specify as or href as keys in these object props: https://github.com/search?q=(org:10gen+OR+org:evergreen-ci+OR+org:mongodb+OR+org:mongodb-js+OR+org:mongodb-labs+OR+org:wiredtiger)+AND+%22%3CInlineMessageFeedback%22+AND+(submitButtonProps%3D+OR+cancelButtonProps%3D)+AND+(%22as:%22+OR+%22href:%22)&type=code
Comment on lines
+6
to
+9
| type ButtonPropsOmittingVariant = Omit<BaseButtonProps, 'variant'>; | ||
| type ButtonPropsWithRequiredChildren = Required< | ||
| Pick<BaseButtonProps, 'children'> | ||
| >; |
There was a problem hiding this comment.
Confirmed that primary consumers' FormFooter instances don't currently specify as or href as keys in these object props: https://github.com/search?q=(org:10gen+OR+org:evergreen-ci+OR+org:mongodb+OR+org:mongodb-js+OR+org:mongodb-labs+OR+org:wiredtiger)+AND+%22%3CFormFooter%22+AND+(primaryButtonProps%3D+OR+backButtonProps%3D+OR+cancelButtonProps%3D)+AND+(%22as:%22+OR+%22href:%22)&type=code
| forwardRef, | ||
| ) { | ||
| const { darkMode } = useDarkMode(darkModeProp); | ||
| export const Button = InferredPolymorphic<ButtonProps, 'button'>( |
| onClick: isInteractive | ||
| ? onClick | ||
| : (e: React.MouseEvent) => e.preventDefault(), | ||
| href: isInteractive ? rest.href : undefined, |
There was a problem hiding this comment.
I can't destructure href from the rest object to allow passing it to useInferredPolymorphic, but I moved this below the ...rest, so it should actually be working now
| ({ children, ...props }: ButtonProps, ref) => ( | ||
| <Button {...props} ref={ref}> | ||
| <Button {...props} as="button" ref={ref}> |
There was a problem hiding this comment.
Paired with Adam on this, and it should be correct now without needing to specify the as prop
shaneeza
reviewed
Feb 7, 2025
| as?: 'a'; | ||
| } & ComponentPropsWithRef<'a'>; | ||
|
|
||
| /** | ||
| * Union of {@link AnchorLikeProps} and {@link InheritedProps} | ||
| */ | ||
| export type InheritedExplicitAnchorLikeProps<TAsProp extends AnchorLike> = { | ||
| /** | ||
| * |
shaneeza
approved these changes
Feb 7, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
Buttonnow usesInferredPolymorphicinstead ofBox<button>🎟 Jira ticket: LG-2719 | LG-3260
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes
Buttonlive example, addhreftext and toggle ondisabled. No browser error relating todisabledelement incompatibility should appear