-
Notifications
You must be signed in to change notification settings - Fork 66
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
Allow defaultChecked to be used in Checkbox #2600
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f4ed28c 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: +37 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
@@ -0,0 +1,5 @@ | |||
--- | |||
'@leafygreen-ui/checkbox': patch |
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 this would be a minor
change, since it adds a feature
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.
Okay!! Should I change it manually to minor
in this file or should I run the script again to change it?
@@ -50,6 +50,7 @@ function Checkbox({ | |||
id: idProp, | |||
indeterminate: indeterminateProp, | |||
label = '', | |||
defaultChecked = false, |
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 believe other components use initialValue
(e.g. Combobox). Unsure whether consistency with other LG components or with HTML checkbox api is more valuable
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.
The Text Area
uses defaultValue. I would vote to use defaultValue
across all components to meet HTML naming rule.
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 don't have a strong opinion; I'll defer to @TheSonOfThomp since you regularly work in this repo.
✍️ Proposed changes
Allows Checkbox to default to true when uncontrolled. Currently, if the checkbox is uncontrolled, it only supports starting in a false state. However, there are cases where we want to have the checkbox be uncontrolled, but start in a true state.
In essence, this would support usage of the defaultChecked property!
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesLet me know if there's anything I can do!! Also, feel free to close if it's not something that's right for leafy green :D it would just be a really useful feature for us on the MANA team!