Rework Group and Inputs #2

xdrdak · 2019-09-13T12:56:10Z

Description

The ever famous Group is about to get kicked to the curb!

Group does some weird recursion thing and strange z-index shenannigans which can lead to some really funky behaviour. So, we're gonna be phasing it out in favour of the new grouping components.

On a different note, I've also tweaked the Input components to export their base components and attempted to simplify the API a bit. The old status object prop still works, but will be removed in the next major version.

InputGroup

Introducing the InputGroup component. Use this component to smoosh input components together (like input, select and buttons).

It works by simply cloning its children and forwards the right border props. This has a few interesting quirks, namely you must keep the structure of the components flat. This also means that, as long as the components have the border props, they can get properly smooshed.

// old
<Group noSpacing>
  <Input />
  <Button>Submit</Button>
</Group>

// new
<InputGroup>
  <Input />
  <Button>Submit</Button>
</InputGroup>

⚠️ Please note that it is not possible to smoosh column groups no longer. Only rows is supported at the moment.

Also important to note, InputGroup does not manipulate anything other than border and border-radii. meaning that it will not automatically attempt to set components to have full width or whatever. This also means that trying to nest things in another component (including fragments) will result in the border properties not being applied properly. This is actually intended to reduce zany side-effects.

⚠️ Please note: By going with this approach, any Groups with a Popover component will NOT have the proper styling applied, due to border props not getting forwarded. Instead, you may want to use the Dropdown component instead, which accomplishes the same thing, but has the appropriate border props applied.

You can resize components by either using flex or width props of those input components.

InputGroupAddon

This component functions similarly to the old GroupAddon component. It's essentially a pre-styled Flex.

// old
<Group noSpacing>
  <GroupAddon>addon</GroupAddon>
  <Input />
  <Button>Submit</Button>
</Group>

// new
<InputGroup>
  <InputGroupAddon>addon</InputGroupAddon>
  <Input />
  <Button>Submit</Button>
</InputGroup>

Input, Radio and Checkbox now export their Base Components

Each of these components export a more primitive version of themselves. This is particularly useful if you need to further customize the component, but don't really want the additional cruft that comes with these components.

Asides from some minor additional props (input prefix and suffix is still present), there's not much magic going on, meaning you are free to re-assemble your form inputs should the need presents itself. And to help you do that, we have a few more helper components inbound.

Input no longer uses objects to represent status and status message

You should no longer pass an object with magical keys to output the status and status message of a text input.

// old
<Input status={{ type: 'danger', message: 'some message'}} />

// new
<Input status="danger" statusMessage="some message"

As a fun little upgrade, statusMessage takes a ReactNode, so feel free to customize your error message!

Label and FormHelper

Label is a new component that takes care of the needed styling and positioning of the input's description field. Use it as you would any other regular label!

<Label htmlFor="myinput" description="Some description">My Label</Label>
<Input id="myinput" placeholder="My input placeholder" />

<RadioLabel htmlFor="myinput" description="Some description">
  <Radio id="myinput" placeholder="My input placeholder" />
  My Label
</RadioLabel>

FormHelper is used to display helper text or status messages. Use it in conjunction when you need to display error messages. You'll need to manually set the status of the component, since it's just a dumb pre-styled text component.

<Label htmlFor="myinput" description="Some description">My Label</Label>
// Input still has a status prop
<Input id="myinput" placeholder="My input placeholder" status="error" />
<FormHelper status="error">
  This is an error message
</FormHelper>

RadioLabel and CheckboxLabel

Radio and Checkbox are somewhat odd ducks, in that the positioning of their description is not aligned to the actual input, but rather the label. Meaning the description is shifted slightly right to align with the label.

I've provided a RadioLabel and CheckboxLabel to get around this particular quirk. Both have the same API and styling. It's simply the name has been adjusted to match their relevant context.

// The only added prop is description. Asides from that, it behaves exactly how a regular html label would
<RadioLabel description="My properly aligned description">
  <BaseRadio />
  My Label
<RadioLabel>

How to test?

Checkout branch, run yarn dev
Open Storybook
Or check the deploy preview on Netlify (link available in comments)

Checklist

I have read the CONTRIBUTING guide
I have prepared CHANGELOGs for release
I have tested my changes on supported browsers
I have added tests that prove my fix is effective or that my feature works

netlify · 2019-09-13T13:00:48Z

Deploy preview for lightspeed-flame ready!

Built with commit f8f5159

https://deploy-preview-2--lightspeed-flame.netlify.com

netlify · 2019-09-13T13:07:26Z

Deploy preview for lightspeed-flame ready!

Built with commit 585f4a5

https://deploy-preview-2--lightspeed-flame.netlify.com

glambert

Grouping Select does not remove border-radius on top and bottom left:

For InputGroup the input container still has the border-radius set on it, so it shows with the box-shadow:

alexislightspeed · 2019-10-17T19:30:25Z

LegacyGroup does this nice thing where the last element you've clicked shows focus on all 4 borders. This Group doesn't seem to have the same behaviour.

alexislightspeed · 2019-10-17T19:47:48Z

FormField is missing a README

alexislightspeed · 2019-10-17T19:55:28Z

InputGroupAddon has the same color as bodyBg. This makes it harder to tell the layers of UI apart. Maybe consider changing that to gray-100 instead of gray-50 ?

@glambert could we have a bg and color prop on InputGroupAddon to allow the manipulation of those colors?

glambert · 2019-10-18T17:48:41Z

InputGroupAddon has the same color as bodyBg. This makes it harder to tell the layers of UI apart. Maybe consider changing that to gray-100 instead of gray-50 ?

@glambert could we have a bg and color prop on InputGroupAddon to allow the manipulation of those colors?

Related to bodyBg, those inputs will always be in a Card tho, correct?

Yes, it would be for edge-cases but we can enable it. We should write a note in the README on that as well 👍

glambert · 2019-10-23T21:24:57Z

LegacyGroup does this nice thing where the last element you've clicked shows focus on all 4 borders. This Group doesn't seem to have the same behaviour.

@alexislightspeed so if we start putting back those z-index "hacks" this always come back to haunt us when people are using those groups and put things inside, such as a DatePicker for example. You'll see that when an input is the first item on the left, the 4 borders will be highlighted correctly on focus. Do we have use-cases where an input is not the first element in a group?

alexislightspeed · 2019-10-24T13:35:03Z

Yes, there are loads of usecases

(for one)
Is it so bad to swap z-indices within InputGroups ?

glambert · 2019-10-24T16:02:23Z

Yes, there are loads of usecases

(for one)
Is it so bad to swap z-indices within InputGroups ?

It creates lots of issues when you start putting Dropdown-type content because InputGroup needs to be relatively positioned to fiddle around z-indices.

The gains of that tiny visual change is minimal compared of the downstream issues we can get by messing with z-index, I'd keep it as is for now and if anything, this can be done on consumer-side which we can provide examples for.

…cing

kvicrey-ls

A few minor comments, otherwise very impressed by the quality of the PR.
I did my best to review carefully the component changes, a bit less on the stories.

I'm going to try the changes on local, but at least for now you can answer my questions.

Great job!! 👍

kvicrey-ls · 2019-10-24T20:33:00Z

packages/flame/src/Checkbox/Checkbox.test.tsx

@@ -12,8 +12,8 @@ type RenderTest = {
    indeterminate?: boolean;
    id?: string;
    value?: string;
-    label?: string | Function;
-    description?: string | Function;
+    label?: any;


string | Function | React.Element?

This is a test file so I don't think its worth 😉

kvicrey-ls · 2019-10-24T20:45:07Z

packages/flame/src/Core/index.tsx

+  borderBottomRightRadius?: string | number;
+}
+
+const borderRadii = system({


Surprised this is not already part of styled-system?

It is, but it's part of the border API which also has a lot more we don't want to enable. Probably why this was created, to limit props you can change to only those needed. To be re-confirmed by @xdrdak.

kvicrey-ls · 2019-10-24T20:54:51Z

packages/flame/src/FormField/README.md

+import { Input } from '@lightspeed/flame/Input';
+
+const MyComponent = () => (
+  <Label htmlFor="myinput" description="Some description">My Label</Label>


It misses <> </> to wrap this series of components.

kvicrey-ls · 2019-10-25T14:29:20Z

packages/flame/src/Checkbox/README.md

+| Prop            | Type                         | Default   | Description                                                    |
+| --------------- | ---------------------------- | --------- | -------------------------------------------------------------- |
+| `description`   | `string` or `child function` | undefined | Description's text                                             |
+| `html property` | `string`                     | undefined | Any extra properties passed will be added to the `<label>` tag |


Not sure I understand html property

Any valid HTML properties such as id, for, etc.

OK, I would add an example then, because it's not very clear.

kvicrey-ls · 2019-10-25T14:36:27Z

packages/flame/src/InputGroup/InputGroup.tsx

+`;
+
+const InputGroup: React.FC = ({ children, ...restProps }) => {
+  const nextChildren = React.Children.map(children, (child: any, index) => {


any => React.Element?

Usually there is a good reason @xdrdak uses any for those. I'd keep it as is and fix in a later PR.

Maybe sometimes it's only a text, in such case I'm not sure it's a React.Element. But we should remove any any in the future.

packages/flame/src/Checkbox/README.md

Co-Authored-By: Kevin Vicrey <45831907+kvicrey-ls@users.noreply.github.com>

kvicrey-ls

Let's merge the MPR! 💯 💯

xdrdak added 2 commits September 13, 2019 08:52

Rework Group and Inputs

f8f5159

use styled-system compose function

b880cbb

glambert added breaking change A major version bump will be needed pkg: flame labels Sep 13, 2019

glambert and others added 3 commits September 18, 2019 19:17

Merge branch 'master' into rework-group-and-inputs

27e29eb

Add layout props to select component

7bf24fb

Merge branch 'master' into rework-group-and-inputs

58df4f7

xdrdak marked this pull request as ready for review October 1, 2019 18:41

xdrdak requested a review from a team as a code owner October 1, 2019 18:41

glambert removed the pkg: flame label Oct 1, 2019

xdrdak added 3 commits October 11, 2019 08:40

Merge branch 'master' into rework-group-and-inputs

6f8af22

Added back some legacy API for compat reasons and easier migration

d2423ca

Add back the legacy group for now

987dab2

xdrdak added enhancement New feature or request and removed breaking change A major version bump will be needed labels Oct 11, 2019

xdrdak added 3 commits October 11, 2019 09:48

Tests for legacy API and component

17b278f

Make codecov less cranky

0aff970

Fix checkbox custom component tests

220f930

glambert reviewed Oct 17, 2019

View reviewed changes

xdrdak added 2 commits October 17, 2019 08:48

Add border props to select

6d3ff8b

Add back border properties to InputWrapper

27c5906

xdrdak force-pushed the rework-group-and-inputs branch from 81c7113 to 27c5906 Compare October 17, 2019 13:13

xdrdak added 2 commits October 17, 2019 14:47

Fix input warning story. Force line-height

713f218

Adjust a SpacedGroup to match old vertical alignment

dd4685c

Adjust Checkbox and Radio stories

b655b1b

glambert added 15 commits October 24, 2019 12:09

Enable color props on InputGroupAddon component

b18eb82

Remove SpacedGroup component, keep it for stories purpose only

4c65656

Dont show Input status box in suffix if no status, fix FormHelper spa…

e55aa8c

…cing

Add back flameName for Group component compat

89e0fdd

Update snapshots

eec23d0

Remove text on Checkbox story for better Percy matching

16b2ec2

Remove disable text on FormField, make Label inline-flex

ffd4445

Update snapshots

4541b38

Add Dialog story for Percy

660ed47

Add back display: flex for BaseLabel

e258ad7

Put FormHelper top margin at Input level

9e12c71

Add README for FormField component

a35a4f2

Update CHANGELOG, make sure all base components are exported

014fefd

Renamed Styled System aliases

c2b2d1d

Enable zIndex Styled System props on InputGroup-based components

aea7148

kvicrey-ls reviewed Oct 25, 2019

View reviewed changes

Fix FormField README examples

7054b63

kvicrey-ls reviewed Oct 25, 2019

View reviewed changes

packages/flame/src/Checkbox/README.md Outdated Show resolved Hide resolved

Update packages/flame/src/Checkbox/README.md

7ba4736

Co-Authored-By: Kevin Vicrey <45831907+kvicrey-ls@users.noreply.github.com>

kvicrey-ls approved these changes Oct 25, 2019

View reviewed changes

glambert added 2 commits October 25, 2019 15:29

Merge branch 'master' into rework-group-and-inputs

73be6ab

Update FormField README

585f4a5

glambert merged commit 8655589 into master Oct 25, 2019

glambert deleted the rework-group-and-inputs branch October 25, 2019 19:50

glambert mentioned this pull request Oct 25, 2019

Input and Group refactor #17

Closed

glambert mentioned this pull request Nov 22, 2019

Flame next major #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Group and Inputs #2

Rework Group and Inputs #2

xdrdak commented Sep 13, 2019 •

edited by glambert

netlify bot commented Sep 13, 2019

netlify bot commented Sep 13, 2019 •

edited

glambert left a comment

alexislightspeed commented Oct 17, 2019 •

edited

alexislightspeed commented Oct 17, 2019

alexislightspeed commented Oct 17, 2019 •

edited

glambert commented Oct 18, 2019 •

edited

glambert commented Oct 23, 2019

alexislightspeed commented Oct 24, 2019

glambert commented Oct 24, 2019 •

edited

kvicrey-ls left a comment

kvicrey-ls Oct 24, 2019

glambert Oct 25, 2019

kvicrey-ls Oct 24, 2019

glambert Oct 25, 2019

kvicrey-ls Oct 24, 2019

kvicrey-ls Oct 25, 2019

glambert Oct 25, 2019

kvicrey-ls Oct 25, 2019

kvicrey-ls Oct 25, 2019

glambert Oct 25, 2019

kvicrey-ls Oct 25, 2019

kvicrey-ls left a comment

Rework Group and Inputs #2

Rework Group and Inputs #2

Conversation

xdrdak commented Sep 13, 2019 • edited by glambert

Description

InputGroup

InputGroupAddon

Input, Radio and Checkbox now export their Base Components

Input no longer uses objects to represent status and status message

Label and FormHelper

RadioLabel and CheckboxLabel

How to test?

Checklist

netlify bot commented Sep 13, 2019

netlify bot commented Sep 13, 2019 • edited

glambert left a comment

Choose a reason for hiding this comment

alexislightspeed commented Oct 17, 2019 • edited

alexislightspeed commented Oct 17, 2019

alexislightspeed commented Oct 17, 2019 • edited

glambert commented Oct 18, 2019 • edited

glambert commented Oct 23, 2019

alexislightspeed commented Oct 24, 2019

glambert commented Oct 24, 2019 • edited

kvicrey-ls 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

kvicrey-ls left a comment

Choose a reason for hiding this comment

xdrdak commented Sep 13, 2019 •

edited by glambert

netlify bot commented Sep 13, 2019 •

edited

alexislightspeed commented Oct 17, 2019 •

edited

alexislightspeed commented Oct 17, 2019 •

edited

glambert commented Oct 18, 2019 •

edited

glambert commented Oct 24, 2019 •

edited