feat: upgrade fundamental-styles to 0.4.0-rc.24 #878

jbadan · 2020-02-21T00:15:45Z

Description

I was originally going to upgrade only to 0.4.0-rc.10, but there were no additional breaking changes between 0.4.0-rc.10 and rc-24.

Changes bold denotes breaking change:

FormInput

changed state prop options to match new validation states

Checkbox

new state prop: added validation states

FormRadioGroup

div to Fragment to match html from Fundamental Styles

FormItem

new isHorizontal prop
isInline becomes internal only prop as it is only meant for Checkboxes and Radios

FormRadioItem

new compact prop
new state prop: added validation states
inline prop becomes internal use only - should only be applied to FormRadioGroup

FormSelect

new state prop: added validation states
new compact prop

FormTextarea

new compact, disabled and readOnly props
new state prop: added validation states

Combobox Input

rebuilt with InputGroup
new state prop: added validation states

SearchInput

rebuilt with InputGroup
new state prop: added validation states

InputGroup

new disabled prop
new state prop: added validation states
fixed compact prop so that appropriate fd-input-group__addon—compact class is added to InputGroup.Addon

MultiInput

rebuilt with InputGroup
changed placeHolder prop to placeholder
new state prop: added validation states

FormLabel

reworked required prop
new is-Toggle prop - internal use only
new is-Checkbox prop - internal use only
new is-Radio prop - internal use only
new is-InlineHelp prop - add this to any labels used with InlineHelp component

FormMessage

changed types options: help -> information

TimePicker

rebuilt with InputGroup

Alert

div for text changed to p

Known Issues out of the scope of this pr:

Dropdown missing top button border
InlineHelp doesn’t actually work when paired with FormLabel + FormInput
Spacing inside TimePicker popover with TimeItems
checkboxes and radios styling looks bad, but this is fixed in a later rc

jbadan · 2020-02-21T00:16:17Z

package.json

    "chain-function": "^1.0.1",
    "classnames": "^2.2.6",
-    "fundamental-styles": "~0.4.0-rc.3",
-    "hoist-non-react-statics": "^3.3.0",


hoist-non-react-statics should have been removed with withStyles

netlify · 2020-02-21T00:20:15Z

Deploy preview for fundamental-react ready!

Built with commit f1020d1

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

src/ComboboxInput/__stories__/ComboboxInput.stories.js

src/ComboboxInput/ComboboxInput.js

src/Forms/Forms.Component.js

src/Forms/__stories__/FormRadioGroup.stories.js

jbadan · 2020-02-21T15:04:17Z

Closing this for now - I'm going to upgrade to 21 since a lot of the validation states were removed between 10 and 20, but added back in 21.

skvale · 2020-02-21T16:28:32Z

src/Forms/Checkbox.js

        className,
-        'fd-checkbox'
+        'fd-checkbox',
+        { [`is-${state}`]: state }


I want to entertain this thought: Should we be defining classes with full strings instead of interpolation.

If we use string interpolation we wouldn't work with something like PurgeCSS in case a consumer wants to configure their project to minimize the imported styles.

So we'd want some utility to add the full strings here

{ ['is-warning']: state.warning, ['is-invalid']: state.invalid, ... }

PurgeCSS looks through all the imported CSS and removes the selectors that your app never uses, but it isn't smart enough to know all the values of state that could be included in the string.

I like the idea of this - maybe as a follow up if we want to use purgeCSS in nui-widgets/floorplans. We could go through all the components with these issues.

I also prefer to avoid string interpolation for classnames - they don't show up in search and replaces efforts, which inevitably leads to bugs.

I can see @bcullman's point and in our legacy applications I completely agree. However, I will say that in the world of self-contained styles, I don't think this is nearly as big of an issue anymore.

Let's 🅿️ this today... the more I think about it, the less I want to make this change

jbadan · 2020-02-21T20:06:14Z

src/Forms/FormItem.js

    children: PropTypes.node,
    className: PropTypes.string,
    disableStyles: PropTypes.bool,
+    isHorizontal: PropTypes.bool,


I don't love the name of this prop 🤷‍♀

It looks like isHorizontal puts the label and the input side-by-side on the same visual row. That seems more like isInline to me, but I also see that prop existed before. I'm not quite sure what the difference is between isHorizontal and isInline. Looking at the fundamental-styles page on forms, there are no uses of fd-form-item--inline. 🤷‍♂

--inline is only for inline labels for radio and checkbox. I think we can just go forward like this, then with the rc that changes how checkboxes and radios work can revisit that?

jbadan · 2020-02-21T20:07:04Z

src/Forms/FormLabel.js

+            'is-disabled': disabled,
+            'fd-form-label--toggle': isToggle,
+            'fd-form-label--checkbox': isCheckbox,
+            'fd-form-label--radio': isRadio,


I don't really love this pattern - but checkbox and radio are going to change significantly in a couple more rc rev's so there's no point in really diving into it right now.

greg-a-smith · 2020-02-24T03:17:28Z

src/ComboboxInput/__stories__/ComboboxInput.stories.js

-                </Menu>
-            }
-            placeholder='Select Fruit' />
+            {...createProps()}


Going forward, it would be good to have one "default" story for each component that contains all the storybook knobs, but all additional stories should probably disable knobs as the stories really become specific use cases with all the appropriate props set and shouldn't be "played with" anymore. There are a few exceptions to this, yes, but it would be good if that was the general pattern. That said, not part of this PR.

Agree! I started to do that... then realized it's soooo out of scope. What do you think about a story in the backlog to 1) upgrade storybook to use the "new" pattern we've established in floorplans 2) setup the stories consistently for the components?

Yeah, that sounds like a good plan.

greg-a-smith · 2020-02-24T03:28:50Z

src/Forms/FormItem.js

    children: PropTypes.node,
    className: PropTypes.string,
    disableStyles: PropTypes.bool,
+    isHorizontal: PropTypes.bool,


It looks like isHorizontal puts the label and the input side-by-side on the same visual row. That seems more like isInline to me, but I also see that prop existed before. I'm not quite sure what the difference is between isHorizontal and isInline. Looking at the fundamental-styles page on forms, there are no uses of fd-form-item--inline. 🤷‍♂

greg-a-smith · 2020-02-24T03:31:10Z

src/Forms/FormRadioItem.js

+        'fd-radio',
+        {
+            'fd-radio--compact': compact,
+            [`is-${state}`]: state


Not exactly "self-contained", but that's a WHOLE OTHER issue.

greg-a-smith · 2020-02-24T03:42:36Z

src/Forms/Forms.Component.js

-                description={`Inputs are used to collect data from the user. When a field is required, the label is displayed in bold
-                    and noted by an asterisk (*).`}
+                description={`Inputs are used to collect data from the user. When a field is required,
+                the <code>required</code> prop will include an asterisk (*).`}


I think the playground components are setup to support markdown if you didn't want to hardcode markup tags.

greg-a-smith

Looks good. 🚢

jbadan added 11 commits February 17, 2020 12:23

update fs to 0.4.0-rc.9

467e2de

wip

df5fc0f

merge master

7b5c702

wip

627142a

validation states

d8e89a6

more changes

3d919ab

form message types

37b5ad9

more updates

7e96a21

update tests

be29c69

add new snapshots for new stories

9a1334a

go up to rc 20

0c0777c

jbadan commented Feb 21, 2020

View reviewed changes

jbadan requested review from a team and meganaconley February 21, 2020 00:21

skvale reviewed Feb 21, 2020

View reviewed changes

src/ComboboxInput/__stories__/ComboboxInput.stories.js Outdated Show resolved Hide resolved

skvale reviewed Feb 21, 2020

View reviewed changes

src/ComboboxInput/ComboboxInput.js Show resolved Hide resolved

skvale reviewed Feb 21, 2020

View reviewed changes

src/Forms/Forms.Component.js Show resolved Hide resolved

skvale reviewed Feb 21, 2020

View reviewed changes

src/Forms/__stories__/FormRadioGroup.stories.js Show resolved Hide resolved

jbadan closed this Feb 21, 2020

move forward for validation states

8e72982

jbadan reopened this Feb 21, 2020

jbadan changed the title ~~feat: upgrade fundamental-styles to 0.4.0-rc.20~~ feat: upgrade fundamental-styles to 0.4.0-rc.24 Feb 21, 2020

jbadan requested a review from a team February 21, 2020 16:00

skvale reviewed Feb 21, 2020

View reviewed changes

jbadan commented Feb 21, 2020

View reviewed changes

greg-a-smith reviewed Feb 24, 2020

View reviewed changes

fix docs

f41d37e

This was linked to issues Feb 24, 2020

searchInput should be rebuilt as an InputGroup #389

Closed

Checkbox style looks wired, and looks even worse when set large width and height #826

Closed

Implement Input Group and Button Group in Multi Input Component #529

Closed

greg-a-smith approved these changes Feb 24, 2020

View reviewed changes

merge master

f1020d1

jbadan merged commit abe8d31 into master Feb 24, 2020

jbadan deleted the fix/update-fs branch February 24, 2020 20:08

feat: upgrade fundamental-styles to 0.4.0-rc.24 #878

feat: upgrade fundamental-styles to 0.4.0-rc.24 #878

Uh oh!

Conversation

jbadan commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbadan commented Feb 21, 2020

Uh oh!

skvale Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jbadan commented Feb 21, 2020 •

edited

Loading

netlify bot commented Feb 21, 2020 •

edited

Loading

skvale Feb 21, 2020 •

edited

Loading