-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(PasswordInput): updated size props #10612
chore(PasswordInput): updated size props #10612
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: 70f8310 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6206b0ebb3ed4f0008b6fccb 😎 Browse the preview: https://deploy-preview-10612--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 70f8310 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6206b0eb1cc7e30007d6cd88 😎 Browse the preview: https://deploy-preview-10612--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 70f8310 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6206b0eb1061400007bdbdc5 😎 Browse the preview: https://deploy-preview-10612--carbon-components-react.netlify.app |
onClick = () => {}, | ||
onTogglePasswordVisibility, | ||
placeholder, | ||
size = 'md', |
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.
sorry, last thing. I thought md
was just for v11? Would we need a ternary here for v10 sizes: small & large?
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.
derp, good call. Should I go empty string since there was no default in v10?
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 guess we can also wait to see if Percy finishes out. Md might already be default since before it was just asking for a string.
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 think there are any styles for md
in v10/v11 since there are no styles for ${prefix}--text-input--md
in /styles
or /components
so I guess in a way the default is md size wise, it's just never called out specifically?
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.
@abbeyhrt Totally. That's what I was assuming as well. The fact the Percy didn't yell at me for setting md
is pretty telling, i think. @joshblack can we get some confirmation on these assumptions?
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.
Personally, I would go for an empty for the size prop since md
doesn't have any meaning but it's also something we can change later on since it wouldn't break anything.
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.
Both of the stories seem to be working as expected, so approving :)
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.
Stories look good!
REF 10295
PasswordInput
size prop based on guidance provided by design. REFdefaultProps
object and moved them to default params. REFTesting
Toggle Password Visibility
story is behaving correctly in the v11 & v10 storybooks and that size props are accurately reflected according to the design guidance.