feat: refactor input group #767

hertweckhr1 · 2019-10-14T20:01:43Z

Description

Refactored Input Group to be a bit more clean and act as wrapper to form input and input group add on.

consumers will now put their own form input and input group addon

Added Storybook examples.

Deleted callbacks.

Will create follow-up issue to create component for up/down number form input.

fixes #518

netlify · 2019-10-14T20:09:28Z

Deploy preview for fundamental-react ready!

Built with commit 0a8b751

https://deploy-preview-767--fundamental-react.netlify.com

src/InputGroup/InputGroup.js

src/InputGroup/_InputGroupAddon.js

src/InputGroup/InputGroup.js

src/InputGroup/__stories__/InputGroup.stories.js

src/_playground/Routes.js

greg-a-smith · 2019-10-14T20:32:51Z

@hertweckhr1 The PR title (and subsequent merge commit message) should still be prefixed with either fix: or feat: (I think fix: in this case). The BREAKING CHANGE: is added when merging in the description (not title) field as the first line. Having the breaking change text in the PR title and merge commit title won't do anything with standard version.

greg-a-smith · 2019-10-14T20:35:46Z

src/InputGroup/InputGroup.js

+                actions: actions,
+                addon: addon,
+                compact: compact,
+                className: (child.type === FormInput) ? inputClasses : '',


I wouldn't match on FormInput. I would match on InputGroupAddon and then put the inputClasses on anything that didn't match that. Keep in mind, the "input" here also needs to support a FormTextarea.

greg-a-smith · 2019-10-14T20:36:50Z

src/InputGroup/InputGroup.js

+            <div
+                {...props}
+                className={inputGroupClasses}
+                compact={compact}>


Placing compact on a <div> is not only not going to work, but will throw a console warning as an unexpected DOM attribute.

src/InputGroup/InputGroup.js

greg-a-smith · 2019-10-14T20:40:04Z

src/InputGroup/_InputGroupAddon.js

+            <Icon
+                disableStyles={disableStyles}
+                glyph={glyph}
+                role='presentation' />


This seems wrong. I think the consumer should own what Icon or other children are within the InputGroupAddon. The only "extra" thing we should need is knowing when the child is a button (or children are buttons).

greg-a-smith · 2019-10-14T20:41:08Z

src/InputGroup/_InputGroupAddon.js

+
+InputGroupAddon.propTypes = {
+    actions: PropTypes.bool,
+    addon: PropTypes.oneOf(INPUT_GROUP_ADDON_TYPES),


Might be able to just get away with a boolean isButton prop. There's really nothing special we would do with any other children.

markpalfreeman · 2019-10-14T21:10:05Z

src/InputGroup/InputGroup.Component.js

                        </FormItem>
                    </FormGroup>
                    <br />
                    <FormGroup>
                        <FormLabel>Right Aligned Text Addon</FormLabel>
                        <FormItem>
-                            <InputGroup
-                                addon='€'
-                                addonPos='after'


I hate weird abbreviations anyway! 🎉

markpalfreeman · 2019-10-14T21:11:49Z

src/InputGroup/InputGroup.js

    render() {
-        const {
+        let {


Can we change this back so we're not modifying children props below?

Agreed -- should be const.

markpalfreeman · 2019-10-14T21:12:52Z

src/InputGroup/InputGroup.js

-            'fd-input-group__addon',
-            [{ 'fd-input-group__addon--button': !!actions || inputType === 'number' }]
-        );
-
        const inputClasses = classnames(


nit: this probably doesn't need to call classnames anymore if it's one static string.

markpalfreeman · 2019-10-14T21:13:36Z

src/InputGroup/InputGroup.js

+        const inputGroupClasses = classnames(
+            className,
+            'fd-input-group',
+            [{ 'fd-input-group--compact': compact }]


nit: you shouldn't need the redundant [] (just the object should work).

markpalfreeman · 2019-10-14T21:18:19Z

src/InputGroup/InputGroup.js

-                    inputClasses,
-                    'fd-input--no-number-spinner'
-                );
+        children = React.Children.map(children, (child) => {


Modifying (resetting) props variables in React components always makes me nervous, even just so we know exactly what we're dealing with with no side effects. Can we either:

make a new variable for this to designate it's a newly created map (something like childrenWithProps?)

just do the React.children.map() inline inside the <div> (doesn't necessarily have to be a variable up here)

markpalfreeman · 2019-10-14T21:22:01Z

src/InputGroup/InputGroup.js

    }
 }

+InputGroup.Addon = InputGroupAddon;
+InputGroup.Input = FormInput;
+
 InputGroup.displayName = 'InputGroup';

 InputGroup.propTypes = {
    actions: PropTypes.bool,


@SAP/fundamental-react-team Would anyone be in favor of renaming this while we have a breaking change on our hands?
IMO this would be clearer as hasActions; right now it sounds like it should take an array of actions.

OR do we need actions at all? Is this always just related to "is this thing a button?" 😛

no actions for the winnnnnn 🏅

markpalfreeman · 2019-10-14T21:24:56Z

src/InputGroup/InputGroup.js

-                );
+        children = React.Children.map(children, (child) => {
+            return React.cloneElement(child, {
+                actions: actions,


nit: you don't need to list these twice if the key and variable names match:

{ actions, addon, compact }

src/InputGroup/InputGroup.test.js

jbadan · 2019-10-15T16:20:29Z

src/InputGroup/InputGroup.test.js

+        it('should allow data attribute to be passed to span element', () => {
+            const element = setup({
+                children: [<InputGroup.Input />, <InputGroup.Addon>@</InputGroup.Addon>],
+                'data-sample': 'Sample'


The way this is setup, shouldn't this data-sample be being passed to the input group, not the addon?

ah ok - the more I look at this - the test is right - just the description should be updated.

I think this got lost in the shuffle - can you update the "it" block - it's not passing to the span anymore.

jbadan · 2019-10-15T16:23:17Z

src/InputGroup/InputGroup.test.js

-        test('should allow props to be spread to the InputGroup component for type number\'s up button element', () => {
-            const element = mount(<InputGroup inputType='number' numberUpButtonProps={{ 'data-sample': 'Sample' }} />);
+    describe('Rendering with Props', () => {
+        it('should not throw error if child.type is undefined', () => {


Should we add an additional test for adding the fd-input-group__input logic?

src/InputGroup/_InputGroupAddon.js

jbadan

Just some small changes - InputGroup.Input, eslint-disable and the comment cleanup! Otherwise it's looking great 🎉

src/InputGroup/InputGroup.Component.js

jbadan · 2019-10-15T16:58:59Z

One more: the styling seems off - looks like we're missing a class on the buttons fd-input-group__button

greg-a-smith

I didn't review must in the tests or the stories since some of my other comments may change how those work anyway.

greg-a-smith · 2019-10-15T19:17:42Z

src/_playground/Routes.js

-                                    inputProps={{ 'aria-label': 'Search' }}
-                                    inputValue={this.state.query}
-                                    onChange={this.onChangeHandler} />
+                                <InputGroup type='icon'>


InputGroup doesn't have a type prop.

greg-a-smith · 2019-10-15T19:18:49Z

src/_playground/Routes.js

+                                        <Icon glyph='search' />
+                                    </InputGroup.Addon>
+                                    <FormInput
+                                        onChange={this.onChangeHandler} placeholder='Search'


Very minor nit: when stacking props on multiple lines, there should only be one per line -- placeholder should be on its own line.

greg-a-smith · 2019-10-15T19:33:19Z

src/InputGroup/InputGroup.js

    render() {
-        const {
+        let {


Agreed -- should be const.

greg-a-smith · 2019-10-15T19:34:03Z

src/InputGroup/InputGroup.js

-            'fd-input-group__addon',
-            [{ 'fd-input-group__addon--button': !!actions || inputType === 'number' }]
+            'fd-input-group',
+            { 'fd-input-group--compact': compact }


The class fd-input-group--compact doesn't exist. Child components have the "compact" variations.

greg-a-smith · 2019-10-15T19:35:12Z

src/InputGroup/InputGroup.js

-            addonClassNames,
-            addonPos,
-            addon,
+        let {
            children,
            className,
            compact,
            disableStyles,


We should prune customStyles here as well so it doesn't get spread to the top-level <div>.

greg-a-smith · 2019-10-15T19:38:11Z

src/InputGroup/InputGroup.js

-        up: 'Value for aria-label on the up <button> element.'
-    },
-    numberDownButtonProps: 'Additional props to be spread to the down `<button>` element (for inputType=\'number\').',
-    numberUpButtonProps: 'Additional props to be spread to the up `<button>` element (for inputType=\'number\').'


Lots of 🔥 and I love it! 👍

greg-a-smith · 2019-10-15T19:41:12Z

src/InputGroup/InputGroup.js

-    actions: PropTypes.bool,
-    addon: PropTypes.string,
-    addonClassNames: PropTypes.string,
-    addonPos: PropTypes.oneOf(INPUT_GROUP_ADDON_POSITIONS),
    children: PropTypes.node,
    className: PropTypes.string,
    compact: PropTypes.bool,


We have a choice to make regarding compact and I think there are merits for each approach. If we take a compact prop here, it is simply to pass along to its children (and subsequently through InputGroupAddon to its children) the prop so the proper classes get added to the proper elements. The other option would be to not take the prop here and just have the consumer pass their compact prop to their respective inputs and buttons manually.

I've kept the prop for now to get it simple for users based on team feedback, but can be up for further debate.

greg-a-smith · 2019-10-15T19:42:53Z

src/InputGroup/_InputGroupAddon.js

+        addonClassNames,
+        'fd-input-group__addon',
+        { 'fd-input-group__addon--button': isButton },
+        { 'fd-input-group__addon--button--compact': compact && isButton }


The class fd-input-group__addon--button--compact doesn't exist so this can be removed. See comment in InputGroup.js about how to handle compact prop.

greg-a-smith · 2019-10-15T19:44:37Z

src/InputGroup/_InputGroupAddon.js

+InputGroupAddon.displayName = 'InputGroup.Addon';
+
+InputGroupAddon.propTypes = {
+    addonClassNames: PropTypes.string,


There no need for addonClassNames -- these are for this component so we can just use className.

greg-a-smith · 2019-10-15T19:46:41Z

src/InputGroup/_InputGroupAddon.js

+    addonClassNames: PropTypes.string,
+    children: PropTypes.node,
+    className: PropTypes.string,
+    compact: PropTypes.bool,


There are no defined CSS classes for a "compact" addon. This should be passed to its children (if we keep the prop). See comment in InputGroup.js about how to handle compact prop.

greg-a-smith

Looking much better! Just a few more things to clean up. I haven't reviewed much of the tests yet, but will do so after the next round of edits.

greg-a-smith · 2019-10-16T03:08:33Z

src/InputGroup/_InputGroupAddon.js

+}) => {
+
+    const addonClasses = classnames(
+        'fd-input-group__addon',


The prop className should also be included in here.

greg-a-smith · 2019-10-16T03:17:45Z

src/InputGroup/InputGroup.js

+            <div
+                {...props}
+                className={inputGroupClasses}>
+                {React.Children.map(children, (child) => (


To be safe, we should use the toArray syntax (like is being used in InputGroupAddon).

{React.Children.toArray(children).map(child => {

Sounds good - what the extra benefit is with using React.Children.toArray(children).map vs. React.Children.map in this case?

I actually don't know we need to do this. React.Children.map already protects against null or undefined, and so this feels like redundancy.

https://reactjs.org/docs/react-api.html#reactchildrenmap

@markpalfreeman There was actually a case that someone found in TabGroup where the React.Children.map didn't protect against null or undefined children, which is where this toArray pattern was introduced. Whether it's actually needed here could be up for debate, but I think this is a safe pattern.

greg-a-smith · 2019-10-16T03:19:00Z

src/InputGroup/InputGroup.js

+                {React.Children.map(children, (child) => (
+                    React.cloneElement(child, {
+                        compact,
+                        className: (child.type.displayname === InputGroup.Addon) ? '' : inputClasses


I don't believe this comparison will ever match -- InputGroup.Addon is a function. I think it should be this:

className: (child.type.displayname === InputGroupAddon.displayName) ? '' : inputClasses

className: (child.type.displayname === InputGroupAddon.displayName) ? '' : inputClasses does not work but className: (child.type.displayName === 'InputGroup.Addon') ? '' : inputClasses does 👍

greg-a-smith · 2019-10-16T03:27:53Z

src/InputGroup/_InputGroupAddon.js

+};
+
+InputGroupAddon.propDescriptions = {
+    isButton: 'Set to **true** if add on is button.'


Minor nit: put an "a" before "button".

greg-a-smith · 2019-10-16T03:29:32Z

src/InputGroup/_InputGroupAddon.js

+    isButton: PropTypes.bool
+};
+
+InputGroupAddon.propDescriptions = {


I think the compact prop description should be overwritten here since we are accepting compact at the InputGroup level and the consumer really shouldn't be passing it here (although nothing bad would happen if they did).

compact: '_INTERNAL USE ONLY._',

greg-a-smith · 2019-10-16T03:46:50Z

src/InputGroup/__stories__/InputGroup.stories.js

+                    compact
+                    glyph='navigation-down-arrow'
+                    option='light' />
+            </InputGroup.Addon>


A couple things I noticed:

When the displayName comparison is fixed in InputGroup, these will start working better.

Examples that are using buttons need to have the isButton prop set on InputGroup.Addon.

The compact prop can be removed from the Button components.

I see a few of the examples using knobs for the compact prop on InputGroup -- I think all of these should have that.

compact has been added to all example for stories, except for TextArea.

…yName

greg-a-smith

Very close. Just one last request. I think we should add additional snapshot testing for InputGroupAddon. Something that exercises both the compact and isButton props.

hertweckhr1 · 2019-10-16T21:49:09Z

Very close. Just one last request. I think we should add additional snapshot testing for InputGroupAddon. Something that exercises both the compact and isButton props.

@greg-a-smith -> can do so for isButton. Since compact is being passed down from inputgroup, isn't this shown in the inputgroup snapshot tests that already include snapshots for compact?

greg-a-smith

This looks great!! 🚢

Just remember this is a breaking change, but since this is a 0th version library, we don't actually mark it with "BREAKING CHANGE:" (we don't want a 1.0.0 yet). Instead, change the prefix from fix: to feat: and make sure that same change appears in the commit message title when merging.

jbadan

Just a couple cleanup items in the tests and we're good to 🚀

jbadan · 2019-10-15T21:22:59Z

src/InputGroup/InputGroup.test.js

+        it('should allow data attribute to be passed to span element', () => {
+            const element = setup({
+                children: [<InputGroup.Input />, <InputGroup.Addon>@</InputGroup.Addon>],
+                'data-sample': 'Sample'


I think this got lost in the shuffle - can you update the "it" block - it's not passing to the span anymore.

jbadan · 2019-10-17T15:47:52Z

src/InputGroup/InputGroup.test.js

+
+    describe('Default Rendering', () => {
+        let element = setup({
+            children: [<FormInput />, <InputGroup.Addon>@</InputGroup.Addon>]


Is this necessary? Your setup on line 143 already has children in it. I would either edit the setup to not include any default children or edit your other tests to remove this and just call let element = setup()

jbadan

🚢 ⭐️ 🚀

hertweckhr1 added 3 commits October 10, 2019 19:21

initial refactor

5e3e556

InputGroup.Addon and InputGroup.Input

80a2e87

tests and storybook examples

2aeb2fc

hertweckhr1 requested a review from a team October 14, 2019 20:03

jbadan reviewed Oct 14, 2019

View reviewed changes

src/InputGroup/InputGroup.js Outdated Show resolved Hide resolved

jbadan reviewed Oct 14, 2019

View reviewed changes

src/InputGroup/_InputGroupAddon.js Outdated Show resolved Hide resolved

jbadan reviewed Oct 14, 2019

View reviewed changes

src/InputGroup/InputGroup.js Outdated Show resolved Hide resolved

jbadan reviewed Oct 14, 2019

View reviewed changes

src/InputGroup/__stories__/InputGroup.stories.js Show resolved Hide resolved

jbadan reviewed Oct 14, 2019

View reviewed changes

src/_playground/Routes.js Outdated Show resolved Hide resolved

greg-a-smith requested changes Oct 14, 2019

View reviewed changes

hertweckhr1 changed the title ~~BREAKING CANGE: refactor input group~~ fix: refactor input group Oct 14, 2019

round 1 changes

fad770e

markpalfreeman reviewed Oct 14, 2019

View reviewed changes

hertweckhr1 added 4 commits October 14, 2019 15:10

round 2 no actions, added isButton

ca06ff9

clean up children

bdb1021

iteration on storybook

86cd014

no isbutton bool in stories

3fa0287

hertweckhr1 requested a review from greg-a-smith October 15, 2019 01:07

quick nit fix

fa478fa

jbadan reviewed Oct 15, 2019

View reviewed changes

src/InputGroup/InputGroup.test.js Outdated Show resolved Hide resolved

jbadan reviewed Oct 15, 2019

View reviewed changes

src/InputGroup/_InputGroupAddon.js Outdated Show resolved Hide resolved

hertweckhr1 added 2 commits October 15, 2019 09:29

get rid of inputgroup.input

be61c76

delete custom and disable style reference

c5f5971

jbadan requested changes Oct 15, 2019

View reviewed changes

src/InputGroup/InputGroup.Component.js Outdated Show resolved Hide resolved

displayname, fix children on test, edit component page

ea6d442

hertweckhr1 added 2 commits October 15, 2019 10:22

added test

b7d5fb9

pass fd-input-group__button to button children

01a2fff

greg-a-smith requested changes Oct 15, 2019

View reviewed changes

round 4 delete unneeded --compact classnames

fd6f3f1

hertweckhr1 requested review from jbadan and greg-a-smith October 15, 2019 21:12

greg-a-smith reviewed Oct 16, 2019

View reviewed changes

hertweckhr1 added 2 commits October 16, 2019 10:16

merge with master

00d78b3

round 5, no compact in button, correct matching for child.type.displa…

4e43379

…yName

hertweckhr1 requested a review from greg-a-smith October 16, 2019 18:26

remove hardcoded displayName

e7604cd

greg-a-smith reviewed Oct 16, 2019

View reviewed changes

added test for isButton

827119a

hertweckhr1 requested a review from greg-a-smith October 16, 2019 21:50

hertweckhr1 added 3 commits October 16, 2019 18:43

compact inputaddon snapshot added

67df3b7

fixed snapshot for addon with button

6346ab4

spelling nit

8ad745c

greg-a-smith approved these changes Oct 17, 2019

View reviewed changes

jbadan requested changes Oct 17, 2019

View reviewed changes

edit tests

d4e291c

hertweckhr1 requested a review from jbadan October 17, 2019 16:48

hertweckhr1 added 2 commits October 17, 2019 10:17

delete child.type throw error test

25c16d1

nit on test

0a8b751

jbadan approved these changes Oct 17, 2019

View reviewed changes

hertweckhr1 changed the title ~~fix: refactor input group~~ feat: refactor input group Oct 17, 2019

hertweckhr1 merged commit cc17bfd into master Oct 17, 2019

hertweckhr1 deleted the fix/refactorInputGroup branch October 17, 2019 17:37

feat: refactor input group #767

feat: refactor input group #767

Conversation

hertweckhr1 commented Oct 14, 2019 • edited

Description

netlify bot commented Oct 14, 2019 • edited

greg-a-smith commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbadan left a comment

Choose a reason for hiding this comment

jbadan commented Oct 15, 2019

greg-a-smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-a-smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hertweckhr1 Oct 16, 2019 • edited

Choose a reason for hiding this comment

markpalfreeman Oct 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hertweckhr1 Oct 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hertweckhr1 Oct 16, 2019 • edited

Choose a reason for hiding this comment

greg-a-smith left a comment

Choose a reason for hiding this comment

hertweckhr1 commented Oct 16, 2019

greg-a-smith left a comment

Choose a reason for hiding this comment

jbadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbadan left a comment

Choose a reason for hiding this comment

hertweckhr1 commented Oct 14, 2019 •

edited

netlify bot commented Oct 14, 2019 •

edited

hertweckhr1 Oct 16, 2019 •

edited

markpalfreeman Oct 16, 2019 •

edited

hertweckhr1 Oct 16, 2019 •

edited

hertweckhr1 Oct 16, 2019 •

edited