Skip to content
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(text-input): adds fluid variation of text input #5830

Merged
merged 34 commits into from
Apr 29, 2020
Merged

feat(text-input): adds fluid variation of text input #5830

merged 34 commits into from
Apr 29, 2020

Conversation

aledavila
Copy link
Contributor

@aledavila aledavila commented Apr 9, 2020

References #5416

Adds fluid form component
Fluid form will handle fluid for all of the components in form

Changelog

New

  • fluid text input variation
  • add new FluidForm component

Check text inputs work and look as intended. Check fluid text input matches.

@aledavila aledavila changed the title Fluid inputs [WIP]feat(text-inputs): adds fluid variation of text input Apr 9, 2020
@netlify
Copy link

netlify bot commented Apr 9, 2020

Deploy preview for carbon-elements ready!

Built with commit 13b93d9

https://deploy-preview-5830--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 9, 2020

Deploy preview for carbon-components-react ready!

Built with commit 13b93d9

https://deploy-preview-5830--carbon-components-react.netlify.app

@aledavila aledavila changed the title [WIP]feat(text-inputs): adds fluid variation of text input feat(text-input): adds fluid variation of text input Apr 17, 2020
@aledavila aledavila changed the title feat(text-input): adds fluid variation of text input [WIP]feat(text-input): adds fluid variation of text input Apr 17, 2020
@@ -83,8 +91,10 @@ const TextInput = React.forwardRef(function TextInput(
<div className={helperTextClasses}>{helperText}</div>
) : null;

const { isFluid } = useContext(FormContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we can we make this a prop... I imagine usage of context may stem from design guide dictating that fluid is per-form basis. OTOH software designs tend to go further, which is, setting API in different levels (layers). So we can break down "per-form fluid mode" for component implementation, one is a pure mean to change fluid/regular modes (e.g. <TextInput layout="fluid">), another is app-level mean to ensure all form items is the desired mode, e.g.:

const AppTextInput = props => {
  const { useFluid } = useContext(FormContext);
  return <TextInput {...props} layout={useFluid ? 'fluid' : 'regular'} />;
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asudoh we decided to use context because we wanted to avoid having the user set a fluid prop for each individual form element. Since they can only be used in the context of a FluidForm, we wanted to make it as easy as possible for users to create a fluid form, while not allowing much deviation from design.

Copy link
Contributor

@asudoh asudoh Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for the background @tw15egan. I tend to avoid making "syntactic sugar" thing affect system design (UX-level requirement and system design are often different animals), but I at least now see where it came from.

Another approach to achieve such syntactic sugar is introducing <FluidTextInput>.

@aledavila
Copy link
Contributor Author

@aagonzales hey I removed the margin but also removed the button for now. We are slowly building the fluid form since more things need to be added in, which will happen in a separate PR. For now this mainly for the text input.

@tw15egan tw15egan removed their request for review April 21, 2020 17:53
@aledavila aledavila requested a review from dakahn April 21, 2020 18:00
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preview link isn't working but since this PR is just about text input I'll go ahead and 👍

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks and sorry for getting to this so late 🏄

@aledavila aledavila merged commit 7dc601f into carbon-design-system:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants