-
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
feat(FluidTextInput): create unstable__FluidTextInput
component
#11971
feat(FluidTextInput): create unstable__FluidTextInput
component
#11971
Conversation
✅ Deploy Preview for carbon-components-react ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site 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.
This looks good so far! A few thoughts below.
Previously we had said the reasons for breaking something out a new component would be if it not only modifies styles but also behavior or accessibility. What behavior would be different between TextInput
and FluidTextInput
? Could we write separate tests to cover FluidTextInput
specific behavior or would TextInput
tests cover it?
Overall, I do think the conflict of props between fluid vs. non-fluid support these being their own component.
Having all fluid inputs as their own components has a few advantages I can think of too:
- All fluid input stories will be co-located in the storybook left panel
- Each fluid input will have their own "folder" and set of stories that will be snapshotted and tracked with VRT/Percy.
- Makes it more clear for consumers the API difference between Fluid vs non-fluid
Do you think the mismatch between component name vs style file is a problem? In order to use FluidTextInput
the user has to ensure text-input.scss
styles are included. Conversely, I don’t think we’d want to duplicate all the styles in text-input.scss
to a new fluid-text-input.scss
, so the existing situation is probably fine.
What should we do with the existing isFluid
props out there? Deprecate them with a note to use the new component could be a good first step.
I agree with this in most circumstances, but since this seems to be a bundle of new components and not just a one-off variant, I tend to lean towards bunching them together. You've accurately summarized my thoughts on the advantages of creating a new component, and to me, they outweigh the negatives. This should also be coordinated with Design to ensure they are on the same page and that they are planning to bundle Fluid components together, rather than have them co-exist inside the
I hadn't thought of that; that could definitely be seen as confusing. We could make a new
I think that makes sense. Once this is merged (or included in this PR), we can mark the |
577b34a
to
af80a10
Compare
@tw15egan Nice! That sounds like a good idea. Could include it here, or do it in a separate PR. I added it as an item on #11785 I also made #12003 for the |
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.
All the styles look good to me!! 🎉
af80a10
to
2e08163
Compare
@tay1orjones you may want to check it over again (the last two commits). In #12003 I realized it was wrapping each Instead of scoping styles to |
2e08163
to
8d56d41
Compare
All the styles still look good 👍 |
packages/react/src/index.js
Outdated
@@ -66,6 +66,7 @@ export FileUploader, { | |||
export { FilterableMultiSelect } from './components/FilterableMultiSelect'; | |||
export Form from './components/Form'; | |||
export FluidForm from './components/FluidForm'; | |||
export FluidTextInput from './components/FluidTextInput'; |
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 we want these to be experimental from the start
export FluidTextInput from './components/FluidTextInput'; | |
export FluidTextInput as unstable__FluidTextInput from './components/FluidTextInput'; |
import './test.scss'; | ||
|
||
export default { | ||
title: 'Components/FluidTextInput', |
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.
title: 'Components/FluidTextInput', | |
title: 'Experimental/unstable__FluidTextInput', |
From a quick search, it looks like some story files include the unstable__
prefix, and others don't. Either way is good with me. FluidForm
does not.
eb6329d
to
1feeeba
Compare
1feeeba
to
0ec410c
Compare
unstable__FluidTextInput
component
Refs #11785
Closes #11994
Utilizes existing
FluidForm
component to create a newunstable__FluidTextInput
variant.Changelog
New
unstable__FluidTextInput
component, mimics the current usage of theFluid
TextInput
example, but requires less work for end-users to configureTo Do
FluidTextInput/__tests__/FluidTextInput-test.js
#11994) (will be completed after this is merged in, so we can utilize build(react): add a script that scaffolds out RTL tests #11941)Testing / Reviewing
Go to the `FluidTextInput story and ensure all variants are to spec and render correctly.