-
Notifications
You must be signed in to change notification settings - Fork 77
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
fix: Refactor input group #766
Conversation
Deploy preview for fundamental-react ready! Built with commit 5f1671e |
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 refactor does a nice job of making this component (and sub-components) more React-like. However, I think one of the goals, if not the biggest, was to simplify this component along with making it more React-like. Apologies if that wasn't fully discussed or outlined in the story.
If you step back, the input group is really a very simple component in that it's just a stylized wrapper around an input and an addon (text, symbol, button or a pair of buttons). I think this component should reflect that simplicity.
I don't think InputGroup
should be creating a FormInput
component nor do I think InputGroupAddon
should be creating any Button
components. By doing it this way, we very tightly couple InputGroup
to its children and would require code changes every time there is a new feature request. It also just requires a bunch of top-level props that end up being passed down through all the child components when the consumer could have just specified them directly.
I would see a consumer's use of this component to be more like these examples:
<InputGroup>
<FormInput {...inputProps} />
<InputGroup.Addon type='button'>
<Button {...buttonProps} />
</InputGroup.Addon>
</InputGroup>
<InputGroup>
<InputGroup.Addon>$</InputGroup.Addon>
<FormTextarea {...textareaProps} />
</InputGroup>
Remember, as silly as it sounds, the "dumber" you can make components, the better (usually). The biggest job of the input group components should be to make sure the right CSS classes get added to the right elements.
Also, a lot of the specific implementation details of an input group will be handled by other components in this library. Things like DatePicker
, TimePicker
, ComboBox
, etc. I would also see this "double button" use case for a numeric input field as its own component (doesn't exist yet, but it should). It would be that component's job to define both buttons and handle the up/down actions.
NOTE: I like the addition of the InputGroupAddon
component as a non-exported component, but there should be a static reference made to it on InputGroup
. This way, the consumer gets access to both components by just importing InputGroup
.
InputGroup.Addon = InputGroupAddon;
@greg-a-smith. I have another branch with those exact changes, but wasn’t sure if we wanted to just simplify the code or go the extreme and make breaking changes. I can push up the other changes on Monday and we can decide what we want. |
We are a zeroth version library -- let's 🔥 it down! |
Closing for spec change. |
Description
Refactored Input Group to be a bit more clean.
Added InputGroupAddon as a subcomponent.
Added Storybook examples.
fixes #518