feat(Radio): radio button base implementation #100

alinais · 2018-08-16T18:00:39Z

Radio

A basic radio button.

TODO

API Proposal

Default

<Radio label="Make your choice" />

<div class="ui-radio" type="radio">
  <input type="radio" />
  <label class="ui-label">Make your choice</label>
</div>

codecov · 2018-08-16T18:01:06Z

Codecov Report

Merging #100 into master will decrease coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   88.37%   88.35%   -0.02%     
==========================================
  Files          45       47       +2     
  Lines         757      773      +16     
  Branches      100      109       +9     
==========================================
+ Hits          669      683      +14     
- Misses         85       87       +2     
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Radio/index.ts	`100% <100%> (ø)`
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/Radio/Radio.tsx	`86.66% <86.66%> (ø)`

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 cfd9700...148934b. Read the comment docs.

kuzhelov · 2018-08-20T14:06:49Z

src/themes/teams/components/Radio/radioStyles.ts

+      display: 'inline-flex',
+      position: 'relative',
+      alignItems: 'center',
+      justifyContent: 'flex-end',


could you, please, suggest where this is necessary?

but the justifyContent will be removed from here in next update

kuzhelov · 2018-08-20T14:08:44Z

src/themes/teams/components/Radio/radioStyles.ts

+
+  label: ({ props, variables }): ICSSInJSStyle => {
+    return {
+      alignItems: 'center',


this one is unnecessary (has no effect for this display value)

true, thanks, will fix

kuzhelov · 2018-08-20T14:10:17Z

src/themes/teams/siteVariables.ts

@@ -63,6 +63,8 @@ export const timestampTextColor = gray04
 //
 // Fonts
 //
+export const fontSizeLarge = pxToRem(24)
+export const fontSizeMedium = pxToRem(18)


a bit confused what is the difference between medium and base?

base is smaller than medium - pairing the sizes from current Teams app so that I can provide the example accordingly

will keep the default value of the font-size for now to keep the changes to minimum

kuzhelov · 2018-08-20T14:12:59Z

src/themes/teams/components/Radio/radioVariables.ts

+export default (siteVars: any) => {
+  const vars: any = {}
+
+  vars.fontSize = siteVars.fontSizeMedium


actually, just for the sake of simplifying things a bit - maybe we could just hardcode some value here for now, and return to that later. However, just expressing my thoughts on that - please, feel free to reason by yourself what would be better; nevertheless, in case we would like to leave this changes here, we'd better to make decision about names for fontWeight

I see that the default fontWeight is set to 500 and we need 400 for parity with Teams existing styles.

kuzhelov · 2018-08-20T14:25:14Z

src/components/Radio/Radio.tsx

+
+    return (
+      <ElementType {...rest} className={classes.root} {...htmlInputProps}>
+        {createHTMLInput(input || type, {


while it seems that I do see where this intent goes (and totally support it) with introducing type property that would be a superset of input values - and, at the same time, would be able to extend these values to slider or toggle variants, just for the sake of simplifying things I would rather suggest to omit this type property for now (although, surely, we will need it later).

Primary reason is that currently this logic is a bit misleading - if type is passed as an argument to createHTMLInput, it does mean that all the values that could be passed to type property should be valid values for HTML input one. But, in that case, we might have reasonable question of why we would need two properties that will, effectively, duplicate each other (as it is now - once again, this might change in future) - and, by that leading library's client to confusion about which one should be used from these two, as well as which one will take precedence.

Please, let me know about your thoughts on that.

That is a good point.

I was thinking pretty much on it, whether to add a full new component or to extend the existing Input by providing a different type to the example. I still think that having a different component (Radio in this case) might be easier for the user to understand what she is writing. In the same way we will write a Checkbox component when needed.

However I don't have the full picture on whether the users would like to further create an Input with type 'button', 'color' or 'date' which of course will require different styling and different properties attached. Will need further debate on it for the right implementation here, but until then I will remove the type from the creation of the input, to remove any doubt.

kuzhelov · 2018-08-20T14:27:08Z

src/themes/teams/components/Radio/radioStyles.ts

+  label: ({ props, variables }): ICSSInJSStyle => {
+    return {
+      alignItems: 'center',
+      cursor: 'pointer',


one thing that I am missing with that is an ability to check Radio by clicking on label. Seems that we should do either of two

do not introduce this style property now

or introduce support for radio selection by clicking on its label

will chose the first option for the base implementation

kuzhelov

👍 everything looks nice, have left couple of comments. One thing that I do see really important before merging this is to resolve question around behavior on label click

levithomason · 2018-08-20T22:06:31Z

Regarding the label click, it would be ideal if we can get the input to be a child of the label. This will avoid a lot of complexity in the component that we face over at Semantic UI React.

levithomason · 2018-08-20T22:08:19Z

src/components/Radio/Radio.tsx

+
+  renderComponent({ ElementType, classes, rest, styles }) {
+    const { input, type, label } = this.props
+    const [htmlInputProps, restProps] = this.partitionProps()


Heads up, restProps is unused here and the ElementType below is spreading the rest props received from renderComponent instead.

…compatibility

kuzhelov · 2018-08-22T15:24:06Z

currently experiencing the following problem while trying to run docs from the branch. Easy fix, but needs to be introduced

kuzhelov

please, fix docs issue and it seems that we are good to merge :)

kuzhelov · 2018-08-22T17:11:11Z

actually, almost overlooked Levi's comment about reusing Input component - yes, absolutely agree, we should do it this way.

Unfortunately, currently it is not possible because for the layout that Input component introduces (input element turns out to be wrapped by div). Will create an issue for that and address it asap

alinais requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 16, 2018 18:00

alinais added the 🚀 ready for review label Aug 16, 2018

alinais force-pushed the feature/radio-button-base-implementation branch from 504a834 to 9860779 Compare August 20, 2018 10:54

kuzhelov reviewed Aug 20, 2018

View reviewed changes

levithomason reviewed Aug 20, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Aug 21, 2018

alinais added 4 commits August 21, 2018 16:12

radio button base implementation

0c1e797

updating the changelog

c9d792e

updating Radio variables

de755ab

cleaning the code; moving the input inside of the label for behavior …

174e8cb

…compatibility

alinais force-pushed the feature/radio-button-base-implementation branch from 9860779 to 174e8cb Compare August 21, 2018 17:15

alinais requested a review from miroslavstastny as a code owner August 21, 2018 17:15

removing typings from example

36a1b1f

alinais added comments addressed 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Aug 21, 2018

kuzhelov approved these changes Aug 22, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed comments addressed 🚀 ready for review labels Aug 22, 2018

adding type as propType

8184ed8

alinais added comments addressed 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Aug 22, 2018

small style tweaks

87d7885

Merge branch 'master' into feature/radio-button-base-implementation

e8e262a

kuzhelov removed the comments addressed label Aug 22, 2018

changelog

148934b

kuzhelov merged commit 0dff885 into master Aug 22, 2018

kuzhelov deleted the feature/radio-button-base-implementation branch August 29, 2018 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Radio): radio button base implementation #100

feat(Radio): radio button base implementation #100

alinais commented Aug 16, 2018 •

edited

codecov bot commented Aug 16, 2018 •

edited

kuzhelov Aug 20, 2018

alinais Aug 20, 2018

alinais Aug 21, 2018

kuzhelov Aug 20, 2018

alinais Aug 21, 2018

kuzhelov Aug 20, 2018

alinais Aug 20, 2018

alinais Aug 21, 2018

kuzhelov Aug 20, 2018

alinais Aug 21, 2018

kuzhelov Aug 20, 2018

alinais Aug 20, 2018

kuzhelov Aug 20, 2018

alinais Aug 20, 2018

kuzhelov left a comment •

edited

levithomason commented Aug 20, 2018

levithomason Aug 20, 2018

kuzhelov commented Aug 22, 2018 •

edited

kuzhelov left a comment

kuzhelov commented Aug 22, 2018 •

edited

feat(Radio): radio button base implementation #100

feat(Radio): radio button base implementation #100

Conversation

alinais commented Aug 16, 2018 • edited

Radio

TODO

API Proposal

Default

codecov bot commented Aug 16, 2018 • edited

Codecov Report

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

kuzhelov left a comment • edited

Choose a reason for hiding this comment

levithomason commented Aug 20, 2018

Choose a reason for hiding this comment

kuzhelov commented Aug 22, 2018 • edited

kuzhelov left a comment

Choose a reason for hiding this comment

kuzhelov commented Aug 22, 2018 • edited

alinais commented Aug 16, 2018 •

edited

codecov bot commented Aug 16, 2018 •

edited

kuzhelov left a comment •

edited

kuzhelov commented Aug 22, 2018 •

edited

kuzhelov commented Aug 22, 2018 •

edited