feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props #206

mnajdova · 2018-09-07T14:30:24Z

Radio

This PR introduces the following props to the Radio component: value, disabled, checked, and onChange.

TODO

API Proposal

value

The HTML input value.

disabled

A radio can appear disabled and be unable to change states.

<Radio label="Disabled" disabled />

<div class="ui-radio a b">
  <label class="ui-label j e f k l m i n o p d g h">
    <input type="radio" disabled="" class="c"/>Disabled
  </label>
</div>

onChange

Callback invoked when the radio is clicked.

checked

Whether or not the radio is checked.

 <Radio label="This radio comes pre-checked" checked />

<div class="ui-radio a b">
  <label class="ui-label j e f k l m i n o p d g h">
    <input type="radio" checked="" class="c"/>This radio comes pre-checked
  </label>
</div>

… the Radio component

codecov · 2018-09-07T14:30:29Z

Codecov Report

Merging #206 into master will increase coverage by 0.24%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   91.53%   91.78%   +0.24%     
==========================================
  Files          61       61              
  Lines        1028     1047      +19     
  Branches      155      143      -12     
==========================================
+ Hits          941      961      +20     
  Misses         83       83              
+ Partials        4        3       -1

Impacted Files	Coverage Δ
src/components/Button/ButtonGroup.tsx	`97.29% <ø> (ø)`	⬆️
src/components/Radio/Radio.tsx	`94.11% <91.66%> (+7.45%)`	⬆️
src/components/Label/Label.tsx	`84.44% <0%> (+2.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b4314b...ebe5438. Read the comment docs.

mnajdova · 2018-09-07T14:45:55Z

docs/src/examples/components/Radio/States/index.tsx

+      examplePath="components/Radio/States/RadioExampleChecked"
+    >
+      <Segment style={{ marginTop: '10px' }}>
+        Use{' '}


I stole this from SUIR, I didn't know we can do this. Very cool! :D

jurokapsiar · 2018-09-10T09:02:05Z

Radio group is necessary for accessibility purposes.
It can render just as a div. That will allow us to apply correct aria role and add keyboard handling.
See https://www.w3.org/TR/2017/WD-wai-aria-practices-1.1-20170628/examples/radio/radio-1/radio-1.html

…eact into feat/radio-checked

mnajdova · 2018-09-10T09:40:16Z

@jurokapsiar so according to the post, we should render the RadioGroup and the Radio as div elements with the proper roles. Should I actually replace the implementation with

and style the ::before part to look like a radio button as part of this PR? If this is the final decision about how it should be defined, I would rather do this now then later.

-reverted the Radio to extend UIComponent

levithomason · 2018-09-10T16:24:08Z

We'll pause this work while we first style the radio. This will require an icon or pseudo elements and some custom event handling. Let's make sure that works before designing this next phase, groups.

-styles the Radio button with custom icon

mnajdova · 2018-09-10T19:17:02Z

As per our call, the RadioGroup is stopped for now, but there is an example added of how the buttons can be used as a group. The styles are also updated:

jurokapsiar · 2018-09-11T11:36:12Z

looking into accessibility, but it will take more time to come up with a working solution if we do not want to use default input as is.

looks like the missing thing is handling of the focused state.
let's try to use focused state introduced in #213

If not possible, you can take a look at https://www.w3.org/TR/wai-aria-practices/examples/radio/radio-1/radio-1.html which unfortunately does not work correctly with JAWS screen reader, but is a better starting point.

… icon's margin

kuzhelov · 2018-09-11T15:06:02Z

docs/src/examples/components/Radio/Types/RadioExample.shorthand.tsx

@@ -1,6 +1,10 @@
 import React from 'react'
 import { Radio } from '@stardust-ui/react'

-const RadioExample = () => <Radio label="Make your choice" />
+const handleChange = () => {
+  console.log('The radio checked value was changed!')


probably, it is better to use alert calls here, if the intent is to make it clear to the client that some changes have happened

Makes sense, will change it!

kuzhelov · 2018-09-11T15:06:40Z

src/components/Button/ButtonGroup.tsx

@@ -30,7 +30,7 @@ class ButtonGroup extends UIComponent<Extendable<IButtonGroupProps>, any> {
    /** An element type to render as (string or function). */
    as: customPropTypes.as,

-    /** A button can take the width of its container. */
+    /** The buttons contained inside the ButtonGroup. */


kuzhelov · 2018-09-11T15:10:46Z

src/components/Radio/Radio.tsx

    /** Child content * */
    children: PropTypes.node,

    /** Additional classes. */
    className: PropTypes.string,

+    /** Initial checked value. */
+    defaultChecked: PropTypes.bool,


this one is not used anywhere - should we delete it?

also, it seems that this one is rather pertains to RadioGroup concept, rather than to individual Radio

Every auto-controlled prop must have a default prop associated to it, that is why we have it. I may need to provide example usage of it.

@kuzhelov , I added the usage of the default checked value. This value is intended to be used as a initial check value of the radio button. As the component is auto-controlled, the user should be able to initially set the value of the checked property. Please comment if you have some more doubts on this.

ok - but in that case let me share what my concerns are about. Now we have this defaultChecked to be defined in prop types, but there is no such a property in the component's TS interface - with that it, essentially, means that me as a client is not able to safely specify value for it in the TS code (I mean, this prop won't be advertised as one that is supported by the component, so I could provide some string value, for example).

Sorry, that is my fault, the defaultChecked prop SHOULD be provide in the TS interface. Thanks for the catch!

mnajdova · 2018-09-11T15:25:55Z

The current implementation of the Radio and the example of the group regarding accessibility are behaving the same as the https://www.w3.org/TR/wai-aria-practices/examples/radio/radio-1/radio-1.html The next thing that should be implemented is having the focused border around the buttons. Should we immediately check the button if it is focused, or should be have the same implementation as in the W3 example? What's the proposal for further checks/fixes?

kuzhelov · 2018-09-11T15:32:20Z

src/components/Radio/Radio.tsx

+    }
+
+    if (onChange) {
+      onChange(e, { ...this.props, checked: !checked })


worrying about the fact that this callback method is invoked before state change will be applied to component - in case of potential error in the logic provided by the callback state change won't be applied

Agreed, will move it before invoking the callback.

kuzhelov · 2018-09-11T15:40:48Z

src/components/Radio/Radio.tsx

+
+  private handleChange = (e: React.SyntheticEvent) => {
+    const { onChange, disabled } = this.props
+    const { checked } = this.state


it would be more robust to obtain this flag directly from e.target.checked - due to async state updates there is a room (although very small one) for the state being out of sync with the component's visual look

Makes sense. Will change it!

kuzhelov

couple of comments that we could discuss/address, other than that everything is 👍

-addressed comments in the PR

mnajdova · 2018-09-11T17:09:22Z

As the accessibility aspect of the radio group are pretty complicated, this PR will address ONLY the properties that needs to be implemented in the Radio, and then a dedicated PR will be created for the RadioGroup.

miroslavstastny · 2018-09-12T06:46:49Z

src/components/Radio/Radio.tsx

+
+    this.trySetState({ checked })
+
+    if (onChange) {


_.invoke() does the check for you

I replaced this with _.invoke, but I prefer much this type of invoking because I know for sure when clicking the property that it is used and invoke somewhere in the code, and with using the invoke, the name of the callback is just a string.

miroslavstastny · 2018-09-12T06:52:22Z

src/components/Radio/Radio.tsx

+              circular: true,
+              size: 'mini',
+              variables: {
+                color: checked ? variables.checkedIconColor : variables.uncheckedIconColor,


I see you are already discussing this in #207. It would be great if, eventually, we would not need to touch variables in component code.

unfortunately, this makes 'variables defined in theme' story a bit more complicated, as now we might need to get an access to component props there - as it is discussed in #207. This would be quite significant move, and we should consider whether there are any other not-so-radical alternatives. Still, totally agree that we should finally come up with solution that will eliminate this code from component's logic

Exactly, I was not satisfied with having to use the variable in the renderComponent, but as I mention in the other PR we should either be able to define the variables depending on the state and props, or rethink about the API provided so far for the components.

-replaced onChange with _.invoke

# Conflicts: # CHANGELOG.md

-changed conformance test regarding the transparent handling of the events to check that the second argument is object containing all props instead of just all props -changed checked value to be read from state instead of event

-fixed console error for onChange not being provided to the input

-added value, disabled, checked, defaultChecked and onChange props to…

2d0ea16

… the Radio component

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners September 7, 2018 14:30

-updated changelog

804811c

mnajdova added the question Further information is requested, concerns that require additional thought are raised label Sep 7, 2018

mnajdova commented Sep 7, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed needs author changes Author needs to implement changes before merge labels Sep 7, 2018

Merge branch 'master' into feat/radio-checked

efebc5e

manajdov added 2 commits September 10, 2018 11:17

-added name property on the Radio component

fb7eeb4

Merge branch 'feat/radio-checked' of https://github.com/stardust-ui/r…

5067012

…eact into feat/radio-checked

manajdov added 4 commits September 10, 2018 17:24

-introduced RadioGroup component

5a0a25d

-reverted the Radio to extend UIComponent

-changed example in the radio group

484c642

-small fix

1a9bc23

-removed auto controlled props

ef68706

-removed RadioGroup

64275dc

-styles the Radio button with custom icon

mnajdova changed the title ~~feat(Radio): added value, disabled, checked, defaultChecked and onChange props~~ feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props Sep 10, 2018

mnajdova added 🚀 ready for review and removed question Further information is requested, concerns that require additional thought are raised labels Sep 10, 2018

-adjusting the content of the Radio button and added variable for the…

d3b029b

… icon's margin

kuzhelov reviewed Sep 11, 2018

View reviewed changes

kuzhelov approved these changes Sep 11, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Sep 11, 2018

-removed the radio group example

0eb85bd

-addressed comments in the PR

mnajdova added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Sep 11, 2018

miroslavstastny reviewed Sep 12, 2018

View reviewed changes

-added defaultChecked in the TS interface

f2f03ea

-replaced onChange with _.invoke

miroslavstastny approved these changes Sep 12, 2018

View reviewed changes

manajdov added 8 commits September 12, 2018 12:15

Merge branch 'master' into feat/radio-checked

3fcd718

# Conflicts: # CHANGELOG.md

-change the color of the label when the radio is disabled

c0b38cd

-renamed the test for the radio component

ebe5438

-changed conformance test regarding the transparent handling of the events to check that the second argument is object containing all props instead of just all props -changed checked value to be read from state instead of event

Merge branch 'master' into feat/radio-checked

1385456

Merge branch 'master' into feat/radio-checked

fa40450

-added icon slot in the radio's variables

3217db7

-added icon slot in the radio's props interface

68deec0

-fixed console error for onChange not being provided to the input

-fixed the test in the radio

99019a5

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 13, 2018

mnajdova merged commit 28eb18f into master Sep 13, 2018

levithomason deleted the feat/radio-checked branch September 14, 2018 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props #206

feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props #206

mnajdova commented Sep 7, 2018 •

edited

codecov bot commented Sep 7, 2018 •

edited

mnajdova Sep 7, 2018 •

edited

jurokapsiar commented Sep 10, 2018

mnajdova commented Sep 10, 2018

levithomason commented Sep 10, 2018

mnajdova commented Sep 10, 2018

jurokapsiar commented Sep 11, 2018 •

edited

kuzhelov Sep 11, 2018

mnajdova Sep 11, 2018

kuzhelov Sep 11, 2018

kuzhelov Sep 11, 2018

kuzhelov Sep 11, 2018

mnajdova Sep 11, 2018

mnajdova Sep 11, 2018

kuzhelov Sep 12, 2018 •

edited

mnajdova Sep 12, 2018

mnajdova commented Sep 11, 2018 •

edited

kuzhelov Sep 11, 2018

mnajdova Sep 11, 2018

kuzhelov Sep 11, 2018

mnajdova Sep 11, 2018

kuzhelov left a comment

mnajdova commented Sep 11, 2018

miroslavstastny Sep 12, 2018

mnajdova Sep 12, 2018

miroslavstastny Sep 12, 2018

kuzhelov Sep 12, 2018

mnajdova Sep 12, 2018 •

edited

feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props #206

feat(Radio): added value, disabled, name, checked, defaultChecked and onChange props #206

Conversation

mnajdova commented Sep 7, 2018 • edited

Radio

TODO

API Proposal

value

disabled

onChange

checked

codecov bot commented Sep 7, 2018 • edited

Codecov Report

mnajdova Sep 7, 2018 • edited

Choose a reason for hiding this comment

jurokapsiar commented Sep 10, 2018

mnajdova commented Sep 10, 2018

levithomason commented Sep 10, 2018

mnajdova commented Sep 10, 2018

jurokapsiar commented Sep 11, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Sep 11, 2018 • 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

kuzhelov left a comment

Choose a reason for hiding this comment

mnajdova commented Sep 11, 2018

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

mnajdova Sep 12, 2018 • edited

Choose a reason for hiding this comment

mnajdova commented Sep 7, 2018 •

edited

codecov bot commented Sep 7, 2018 •

edited

mnajdova Sep 7, 2018 •

edited

jurokapsiar commented Sep 11, 2018 •

edited

kuzhelov Sep 12, 2018 •

edited

mnajdova commented Sep 11, 2018 •

edited

mnajdova Sep 12, 2018 •

edited