-
Notifications
You must be signed in to change notification settings - Fork 114
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
chore: fix reset button types #3454
Conversation
π¦ Changeset detectedLatest commit: 272e7b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
βοΈ Nx Cloud ReportCI is running/has finished running commands for commit 272e7b0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. π See all runs for this branch β Successfully ran 1 targetSent with π from NxCloud. |
Size Change: 0 B Total Size: 1.01 MB βΉοΈ View Unchanged
|
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.
perfect
β Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
β Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 272e7b0:
|
Passing run #6693 βοΈDetails:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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.
Ah I had a feeling a few spots must have been missed in my first approval. I should have dug into that hunch.
.changeset/brave-gorillas-begin.md
Outdated
'@twilio-paste/core': patch | ||
--- | ||
|
||
Removal of ts annotation to expect an error for reset button style overrides |
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.
Removal of ts annotation to expect an error for reset button style overrides | |
[User Dialog] Removal of ts annotation to expect an error for reset button style overrides |
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.
Although I wonder if we even need to ship this as a patch since it doesn't affect users really?
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.
Probably not, it's just to appease the Danger JS check
variant?: 'reset'; | ||
}; | ||
|
||
export type ButtonProps = (BaseVariantsButtonProps | ResetVariantButtonProps) & |
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.
Do we want to add docs for this? An example of using reset button? Or keep it under wraps for now?
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.
Keep it under wraps for now
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.
suggestion(non-blocking): can you add to the docs example explaining that our tokens can be used to style reset buttons?
Description and useful links to discussions / issues / tickets
Changes in this PR:
Fix the reset variant button types
Conditionally allow style props on a reset button variant
Improve button types
ButtonProps didn't extend DirectButtonProps. This led to a mismatch between the consumer types and the internal types. For example, to a consumer we didn't restrict tabIndex to 0 and -1 but we did internally.
Iput and User Dialog removal of ts expect error
No longer needed for the reset button as style props are valid.
Checklist
required
checks have passedπ΅π»ββοΈ Run website visual regression
label