feat: add RadioField component, rework RadioInput #537

emmenko · 2019-02-13T16:40:32Z

This PR aims to rework the RadioInput component API to be consistent with the other input components, as well as to add the RadioField component.

There are a couple of extra changes though, as we decided for example to remove some default styling on the Text components in order to make it easier to apply e.g. some colors on the text.
This became particularly obvious in the radio input options.

src/components/fields/radio-field/README.md

tdeekens · 2019-02-13T16:45:41Z

src/components/styles-provider/styles-provider.js

+      `}
+    />
+    {props.children}
+  </React.Fragment>


To by using the RadioField you "import" the side effect of a global style?

montezume

I'm not really sure I agree with the StylesProvider, or at least with the idea of it being part of this PR. I don't see that it adds any more value that the users just applying that CSS themselves...

montezume · 2019-02-13T20:29:52Z

src/components/styles-provider/README.md

+
+| Props          | Type               | Required | Values | Default | Description                                                                                      |
+| -------------- | ------------------ | :------: | ------ | ------- | ------------------------------------------------------------------------------------------------ |
+| `baseFontSize` | `PropTypes.string` |    -     | -      | `1rem`  | Defines the font size baseline from which the `rem` font sizes of the application are based from |


It doesn't make sense to set baseFontSize to 1rem. It should be a pixel value. Maybe name it something like bodyFontSize or htmlFontSize to imply that it's being set on the body...

Sure, I wanted to discuss this with you tomorrow.

montezume · 2019-02-13T20:33:35Z

I labelled it a breaking change because removing margin, color and font-family from the text components is a breaking visual change.

emmenko · 2019-02-13T20:36:55Z

Yes thanks, I didn't finish updating the PR description etc. It should be a breaking change of course.

tdeekens · 2019-02-14T10:11:56Z

src/components/fields/radio-field/README.md

+
+#### Description
+
+A controlled radio input component with validation states and a label.


This still feels off?

Hmm, that's the same as other field components

Okay then. Just read "radio input component" and was like but it's a "radio field component".

tdeekens · 2019-02-14T10:13:54Z

src/components/fields/radio-field/radio-field.spec.js

+  it('should disable the input', () => {
+    const { getByLabelText } = renderRadioField({ isReadOnly: true });
+    expect(getByLabelText(/RadioField Option 1/)).toHaveAttribute('readonly');
+    expect(getByLabelText(/RadioField Option 2/)).toHaveAttribute('readonly');


Are you sure we need this fuzzy matching everywhere? I understand the intend to have tests potentially more stable/flexible but I don't feel case for it really exists?

I simply like it more like this 🙃 Do you see any problems with this approach?

tdeekens · 2019-02-14T10:15:47Z

src/components/inputs/radio-input/radio-group.js

+    if (this.props.direction === 'inline') {
+      return (
+        <div id={this.props.id}>
+          <Spacings.Inline


Not here but maybe in the future. I like the notion of components: {} we recently adopted far more. So it would be components: { spacing: <Spacings.Inline scale='' /> } maybe. Is that worth of something to track as an issue?

I see your point. In this case though, you really only have 2 choices: stack or inline. I'm not sure having the freedom of passing any component would make sense here 🤔

Yeah, me neither fully. Just wanted to share the thought. One could argue (not saying has to) that having an consistent/expected API is also nice. You should go "How can I customise this?" "Ah, just like everywhere, maybe pass in the components thing.".

src/components/fields/radio-field/README.md

montezume · 2019-02-14T10:45:08Z

src/components/fields/radio-field/README.md

+| `value`                | `string`           |    ✅    | -                       | -                | Value of the input component.                                                                                                                                                                                                                                         |
+| `onChange`             | `func`             |    -     | -                       | -                | Called with an event containing the new value. Required when input is not read only. Parent should pass it back as `value`.                                                                                                                                           |
+| `onBlur`               | `func`             |    -     | -                       | -                | Called when input is blurred                                                                                                                                                                                                                                          |
+| `onFocus`              | `func`             |    -     | -                       | -                | Called when input is focused                                                                                                                                                                                                                                          |


Is this right? Or it's called when any of the RadioOptions are focused?

I've put it on the each RadioInput.Option. Do you think we should leave them out (onBlur/onFocus) or should we keep them like this?

montezume · 2019-02-14T10:45:11Z

src/components/fields/radio-field/README.md

+| `name`                 | `string`           |    -     | -                       | -                | Used as HTML `name` of the input component. property                                                                                                                                                                                                                  |
+| `value`                | `string`           |    ✅    | -                       | -                | Value of the input component.                                                                                                                                                                                                                                         |
+| `onChange`             | `func`             |    -     | -                       | -                | Called with an event containing the new value. Required when input is not read only. Parent should pass it back as `value`.                                                                                                                                           |
+| `onBlur`               | `func`             |    -     | -                       | -                | Called when input is blurred                                                                                                                                                                                                                                          |


montezume · 2019-02-14T10:45:25Z

src/components/fields/radio-field/README.md

+| `touched`              | `bool`             |    -     | -                       | `false`          | Indicates whether the field was touched. Errors will only be shown when the field was touched.                                                                                                                                                                        |
+| `name`                 | `string`           |    -     | -                       | -                | Used as HTML `name` of the input component. property                                                                                                                                                                                                                  |
+| `value`                | `string`           |    ✅    | -                       | -                | Value of the input component.                                                                                                                                                                                                                                         |
+| `onChange`             | `func`             |    -     | -                       | -                | Called with an event containing the new value. Required when input is not read only. Parent should pass it back as `value`.                                                                                                                                           |


can a RadioGroup be read only?

Maybe, maybe not. I doesn't hurt to have it I suppose, as it's inline with the other inputs/fields.

montezume · 2019-02-14T10:46:17Z

src/components/fields/radio-field/radio-field.spec.js

+});
+
+describe('when disabled', () => {
+  it('should disable the input', () => {


Feels weird though. to me it makes more sense to pass isDisabled to the individual RadioOptions rather than the field.

You usually pass isDisabled to the entire group. In some special cases, you can then disable only specific options by passing isDisabled directly to them.

src/components/inputs/radio-input/radio-group.js

emmenko · 2019-02-14T13:09:39Z

src/components/inputs/radio-input/README.md

-| `children` | `node`   |    ✅    | -                           | -       | The `RadioInput.Option` or another node (mixed children are allowed)              |
-| `value`    | `string` |    -     | -                           | -       | The selected value of the of a `RadioInput.Option` determining its checked status |
-| `onChange` | `func`   |    ✅    | -                           | -       | What will trigger whenever an `RadioInput.Option` is clicked                      |
-| `scale`    | `string` |    -     | ['xs', 's', 'm', 'l', 'xl'] | `m`     | Spacing between options                                                           |


Technically, this is what causes the breaking change, as it's been renamed to directionProps: { scale }

emmenko · 2019-02-14T13:10:46Z

src/components/inputs/radio-input/radio-group.js

-          })}
-        </DirectionWrapper>
+      <div id={this.props.id}>
+        <Constraints.Horizontal constraint={this.props.horizontalConstraint}>


Only the stack mode has the horizontal constraint, as it does not make sense to have it in the inline mode.

emmenko · 2019-02-14T13:12:30Z

src/components/inputs/radio-input/radio-option.js

+    role="radio"
+    aria-checked={props.isChecked}
+    onFocus={props.onFocus}
+    onBlur={props.onBlur}


I decided to put the onBlur/onFocus event handlers here, as it's what makes more sense to me, because the input is hidden and the entire label is clickable.

Not sure if we really need them though...

emmenko · 2019-02-14T13:13:57Z

src/components/inputs/radio-input/radio-option.js

+  <label
+    css={getLabelStyles(props)}
+    role="radio"
+    aria-checked={props.isChecked}


The role and aria-checked are for a11y purposes.
https://www.w3.org/TR/wai-aria/#aria-checked

emmenko · 2019-02-14T13:21:26Z

src/components/inputs/radio-input/radio-option.js

+          color: ${props.isDisabled
+            ? vars.fontColorDisabled
+            : vars.fontColorDefault};
+        `}


So I ended up removing the Text.Body wrapper, as it was just causing problems with the HTML markup (e.g. <div> nested inside a <p>).
The styles to apply are actually just a few, I think it's better this way.

/cc @montezume

emmenko · 2019-02-14T13:22:23Z

Alright, VRTs have been approved by design, I'd be happy to get another round of review here. Thanks!

tdeekens · 2019-02-18T21:18:35Z

I lost track of this with the other PRs in app-kit. Where are we here? Looks like some sort of walkthrough was supposed to happen?

emmenko · 2019-02-19T12:42:35Z

What do you mean "walkthrough"?

tdeekens · 2019-02-19T13:15:50Z

I just read somewhere that Malcolm and you wanted a call. Maybe that happened. Sorry then. I was just wondering how we wanted to proceed...

emmenko · 2019-02-19T14:12:38Z

Ah yeah, I guess we talked about it last week when I was in Berlin

tdeekens

Looking at the code again, this looks good to me know. Is there anything missing from another side: UX, PO or functionality? Asking cause it's not marked as in dev review.

emmenko · 2019-02-19T16:13:55Z

I'll check it with Filip tomorrow (the percy diffs), otherwise there is nothing else to block this.

…omponents

… styles (as replacement of reset.css)

…g global styles (as replacement of reset.css)" This reverts commit d3a9e26.

…hildren

…lly apply styles

emmenko · 2019-02-20T10:00:45Z

@tdeekens @montezume this is now ready to go (VRTs have been approved by design).

Is there anything left from your side?

montezume

Thanks!

tdeekens requested changes Feb 13, 2019

View reviewed changes

montezume reviewed Feb 13, 2019

View reviewed changes

montezume added 💥 Type: Breaking Change Semver: BREAKING labels Feb 13, 2019

tdeekens reviewed Feb 14, 2019

View reviewed changes

emmenko force-pushed the nm-radio-field branch from 3d5d43f to a88ded3 Compare February 14, 2019 10:24

montezume reviewed Feb 14, 2019

View reviewed changes

emmenko changed the title ~~feat: add RadioField component, rework RadioInput, add (experimental) StylesProvider~~ feat: add RadioField component, rework RadioInput Feb 14, 2019

emmenko commented Feb 14, 2019

View reviewed changes

emmenko force-pushed the nm-radio-field branch from ae429a8 to c5b5188 Compare February 19, 2019 15:31

tdeekens reviewed Feb 19, 2019

View reviewed changes

emmenko added the 👨‍👩‍👦‍👦 Status: Dev Review label Feb 19, 2019

emmenko added 6 commits February 20, 2019 10:46

refactor(inputs/radio): to have consistent API with the other input c…

93ff7fc

…omponents

feat: add experimental component <StylesProvider> for applying global…

99acb42

… styles (as replacement of reset.css)

feat: add RadioField component

a79c0ad

docs(fields/radio): update readmes

f8198c7

test(inputs/radio): use unicode chars for fruits

dd5d8f5

test(inputs/radio): remove fruits

7b119b9

emmenko added 7 commits February 20, 2019 10:46

Revert "feat: add experimental component <StylesProvider> for applyin…

f8eac3e

…g global styles (as replacement of reset.css)" This reverts commit d3a9e26.

refactor(inputs/radio): keep Text.Body wrapper to RadioInput.Option c…

d3c7980

…hildren

fix(inputs/radio): wrapper text component to use 100% width

961a587

fix: export RadioField

8b394ab

fix(grid): font-family for VRT

0ae23ba

fix(fields/radio): remove placeholder vrt, add readonly vrt

31f216a

refactor(inputs/radio): do not use text component as a wrapper, manua…

376ee6c

…lly apply styles

emmenko force-pushed the nm-radio-field branch from c5b5188 to 376ee6c Compare February 20, 2019 09:47

tdeekens approved these changes Feb 20, 2019

View reviewed changes

jonnybel approved these changes Feb 20, 2019

View reviewed changes

montezume approved these changes Feb 20, 2019

View reviewed changes

emmenko merged commit c2fcaf7 into master Feb 20, 2019

emmenko deleted the nm-radio-field branch February 20, 2019 10:22


		#### Description

		A controlled radio input component with validation states and a label.

feat: add RadioField component, rework RadioInput #537

feat: add RadioField component, rework RadioInput #537

Conversation

emmenko commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

montezume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Feb 13, 2019

emmenko commented Feb 13, 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

Choose a reason for hiding this comment

emmenko commented Feb 14, 2019

tdeekens commented Feb 18, 2019

emmenko commented Feb 19, 2019

tdeekens commented Feb 19, 2019

emmenko commented Feb 19, 2019

tdeekens left a comment • edited Loading

Choose a reason for hiding this comment

emmenko commented Feb 19, 2019

emmenko commented Feb 20, 2019

montezume left a comment

Choose a reason for hiding this comment

emmenko commented Feb 13, 2019 •

edited

Loading

tdeekens left a comment •

edited

Loading