Repair Radio Checking onChange #15

terrapanda · 2018-08-07T22:31:57Z

Fix Radio Checked Onchange
Change Input to Switch Case Render
Pull Special Case Types from ./Type i.e. Radio

Fixes #14

bucko13 · 2018-08-08T00:07:56Z

src/components/Input/Input.js

-    );
+    switch (type) {
+      case 'radio':
+        return <Radio {...this.props}>{children}</Radio>;


Nice! I like this a lot. Also adds a lot more flexibility for more custom input types in the future 👍

bucko13 · 2018-08-08T00:18:39Z

src/components/Input/Type/Radio.js

+    } = this.props;
+
+    return children.map((option, idx) => (
+      <div key={option} className={className}>


having everything set to option kind of limits the use of this. For example, we might want a descriptive label but a value that maps to an int.

An ideal for this would be something like what semantic-ui does (http://react.semantic-ui.com/addons/radio/#types-radio-group). Unfortunately we don't have anything like Form.Field right now though...

terrapanda · 2018-08-08T00:26:33Z

Okay, I see what you mean. One possibility is that we could change the signature to accept options as both strings and shape restricted objects ['','',''] -> [{option, value},{option, value},{option, value}] to handle your concern in the interim. Will look into it later.

…

On Tue, Aug 7, 2018 at 5:18 PM Buck Perley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/Input/Type/Radio.js <#15 (comment)>: > + render() { + const { + children, + className, + name, + onChange, + placeholder, + style, + theme, + type, + value, + ...otherProps + } = this.props; + + return children.map((option, idx) => ( + <div key={option} className={className}> having everything set to option kind of limits the use of this. For example, we might want a descriptive label but a value that maps to an int. An ideal for this would be something like what semantic-ui does ( http://react.semantic-ui.com/addons/radio/#types-radio-group). Unfortunately we don't have anything like Form.Field right now though... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AR3NqMRlf-KO9lk-WUL2W7l58N4EhB3rks5uOi5ggaJpZM4Vy_Xw> .

bucko13 · 2018-08-08T00:29:43Z

Okay, I see what you mean. One possibility is that we could change the
signature to accept options as both strings and shape restricted objects
['','',''] -> [{option, value},{option, value},{option, value}] to handle
your concern in the interim. Will look into it later.

Yeah, I was thinking the same. Would be nice to have something more similar, but checking if it's an object and then passing it through as props should be a good fix for now 👍

terrapanda · 2018-08-08T18:35:17Z

Radio Input update complete.

Edge case to consider when implementing Form wrapper is that names of inputs must be unique.

Demo:

<form>
  <Input type="radio" name="unique" onChange={function(e) {}}>
    {['yes', 'no', ['maybe', 'any value']]}
  </Input>
</form>

terrapanda · 2018-08-08T18:39:01Z

Improve option handling with tuple and string compatability:

['label', 'label'] [['label', 'any value'], ['label', 'any value']]

bucko13

Is there any reason why you think we should pass in an array of tuples rather than an array objects? It feels a lot clearer to me to have the object properties map to the type of options a radio button would accept. I think you should even be able to just destructure whatever is passed through and let it populate the input element that way.

bucko13 · 2018-08-09T21:07:27Z

src/components/Input/Type/Radio.js

+
+const optTypeCheck = function(option) {
+  if (Array.isArray(option)) return isLimPrim(option[0]) ? 'array' : false;
+  else return isLimPrim(option) ? 'lim-prim' : false;


what is lim-prim?

a subset of primitives -> number and string

Why is the output polymorphic?

bucko13 · 2018-08-09T21:07:30Z

src/components/Input/Type/Radio.js

+                this.handleChecked(idx);
+                onChange(e);
+              }}
+              placeholder={placeholder}


I might be reading this wrong but wouldn't this mean that all radios will have the same placeholder? Also, do radio inputs have placeholders?

Radio does not have a placeholder to my knowledge.

If this component is going to be exclusively for Radio then this should be removed imo.

terrapanda · 2018-08-10T04:19:33Z

I will be adding object compatibility in the next refactor.

bucko13 · 2018-08-10T17:16:02Z

I will be adding object compatibility in the next refactor.
Is there a reason to change the passed prop format a second time? I feel like if we're taking the time to refactor now, we should just implement the format we intend to use from the get go. Otherwise we'll have to support the old as well as the new and make changes to BUI again too.

bucko13 · 2018-08-10T17:12:23Z

src/components/Input/Type/Radio.js

+                this.handleChecked(idx);
+                onChange(e);
+              }}
+              placeholder={placeholder}


If this component is going to be exclusively for Radio then this should be removed imo.

bucko13 · 2018-08-10T17:14:21Z

src/components/Input/Type/Radio.js

+              }}
+              placeholder={placeholder}
+              style={style}
+              type={type}


following on the previous comment, if this is specifically a Radio input, shouldn't type be hard coded? We should harden on either having flexible, general inputs or having the specific type. My guess is this could allow for someone to do <Radio type="number" {...otherProps} /> and use a radio component to render a non radio input.

terrapanda · 2018-08-10T17:31:18Z

Agreed. I’m going to refactor the structure of input and typing on this component. Ultimately I see it as a Select component with options for “radio”, “toggle” and “drop down”. If there is another “select one” type I’m not thinking of, please comment. Thoughts? @bucko13 @tynes

…

On Fri, Aug 10, 2018 at 10:16 Buck Perley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/Input/Type/Radio.js <#15 (comment)>: > + const radioType = optTypeCheck(option); + const optLabel = radioType === 'lim-prim' ? option : option[0]; + const optValue = radioType === 'lim-prim' ? option : option[1]; + + return ( + <div key={`${optLabel}-${idx}`} className={className}> + <label className={`form-label ${theme.input.capitalize}`}> + <input + checked={idx === this.state.checked} + className={theme.input[type]} + name={`${name}`} + onChange={e => { + this.handleChecked(idx); + onChange(e); + }} + placeholder={placeholder} If this component is going to be exclusively for Radio then this should be removed imo. ------------------------------ In src/components/Input/Type/Radio.js <#15 (comment)>: > + const optValue = radioType === 'lim-prim' ? option : option[1]; + + return ( + <div key={`${optLabel}-${idx}`} className={className}> + <label className={`form-label ${theme.input.capitalize}`}> + <input + checked={idx === this.state.checked} + className={theme.input[type]} + name={`${name}`} + onChange={e => { + this.handleChecked(idx); + onChange(e); + }} + placeholder={placeholder} + style={style} + type={type} following on the previous comment, if this is specifically a Radio input, shouldn't type be hard coded? We should harden on either having flexible, general inputs or having the specific type. My guess is this could allow for someone to do <Radio type="number" {...otherProps} /> and use a radio component to render a non radio input. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AR3NqH6pnZnsMo69G7xHV4pxtG6tWGTHks5uPb_dgaJpZM4Vy_Xw> .

bucko13 · 2018-08-10T17:36:52Z

Yeah, I think have more flexible (and better designed) selector components is a good idea.

For what it's worth, this is an example of a design we have for the currently in progress wallet plugin. As you can see it has a pretty custom radio-like input:

Regarding a dropdown, just FYI, @tynes recently added this: https://github.com/bpanel-org/bpanel-ui/blob/master/src/components/Dropdown/Dropdown.js

terrapanda · 2018-08-10T17:46:11Z

We could map an object of label : value pairs, tuples, or strings as children. React doesn’t like objects as child nodes which is why I implemented tuples but we can easily add a data property. I’ll look at the data handling patterns in other components and work on a better design. I whipped this up to troubleshoot our state handling on the previous radio.

…

On Fri, Aug 10, 2018 at 10:31 Benjamin olki ***@***.***> wrote: Agreed. I’m going to refactor the structure of input and typing on this component. Ultimately I see it as a Select component with options for “radio”, “toggle” and “drop down”. If there is another “select one” type I’m not thinking of, please comment. Thoughts? @bucko13 @tynes On Fri, Aug 10, 2018 at 10:16 Buck Perley ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/components/Input/Type/Radio.js > <#15 (comment)>: > > > + const radioType = optTypeCheck(option); > + const optLabel = radioType === 'lim-prim' ? option : option[0]; > + const optValue = radioType === 'lim-prim' ? option : option[1]; > + > + return ( > + <div key={`${optLabel}-${idx}`} className={className}> > + <label className={`form-label ${theme.input.capitalize}`}> > + <input > + checked={idx === this.state.checked} > + className={theme.input[type]} > + name={`${name}`} > + onChange={e => { > + this.handleChecked(idx); > + onChange(e); > + }} > + placeholder={placeholder} > > If this component is going to be exclusively for Radio then this should > be removed imo. > ------------------------------ > > In src/components/Input/Type/Radio.js > <#15 (comment)>: > > > + const optValue = radioType === 'lim-prim' ? option : option[1]; > + > + return ( > + <div key={`${optLabel}-${idx}`} className={className}> > + <label className={`form-label ${theme.input.capitalize}`}> > + <input > + checked={idx === this.state.checked} > + className={theme.input[type]} > + name={`${name}`} > + onChange={e => { > + this.handleChecked(idx); > + onChange(e); > + }} > + placeholder={placeholder} > + style={style} > + type={type} > > following on the previous comment, if this is specifically a Radio input, > shouldn't type be hard coded? We should harden on either having flexible, > general inputs or having the specific type. My guess is this could allow > for someone to do <Radio type="number" {...otherProps} /> and use a > radio component to render a non radio input. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#15 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AR3NqH6pnZnsMo69G7xHV4pxtG6tWGTHks5uPb_dgaJpZM4Vy_Xw> > . >

bucko13 · 2018-08-10T17:49:20Z

Only the label is actually being used as a child, the rest would be props, which works fine for the object structure. The label could then just be pulled out of each object as you map through them. The problem with the tuple is that implementers have to know the right order to add the options in.

terrapanda · 2018-08-10T17:50:05Z

That’s very close to what I have in the next version following the buttons pattern.

…

On Fri, Aug 10, 2018 at 10:36 Buck Perley ***@***.***> wrote: Yeah, I think have more flexible (and better designed) selector components is a good idea. For what it's worth, this is an example of a design we have for the currently in progress wallet plugin. As you can see it has a pretty custom radio-like input: [image: screen shot 2018-08-10 at 10 34 06 am] <https://user-images.githubusercontent.com/4344978/43972436-37cd50e0-9c89-11e8-83c2-29858bdaf9cc.png> Regarding a dropdown, just FYI, @tynes <https://github.com/tynes> recently added this: https://github.com/bpanel-org/bpanel-ui/blob/master/src/components/Dropdown/Dropdown.js — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AR3NqKuOapi-8TLSlI5Zjfi30IDzCItnks5uPcS1gaJpZM4Vy_Xw> .

bucko13 · 2018-08-10T17:51:28Z

Awesome, let’s just implement that from the get go then. The tuples just seem too brittle to me. Doesn’t seem worth it to have to continue supporting them when an object works so much better.

…

On Aug 10, 2018, at 10:50 AM, Benjamin ***@***.***> wrote: That’s very close to what I have in the next version following the buttons pattern. On Fri, Aug 10, 2018 at 10:36 Buck Perley ***@***.***> wrote: > Yeah, I think have more flexible (and better designed) selector components > is a good idea. > > For what it's worth, this is an example of a design we have for the > currently in progress wallet plugin. As you can see it has a pretty custom > radio-like input: > [image: screen shot 2018-08-10 at 10 34 06 am] > <https://user-images.githubusercontent.com/4344978/43972436-37cd50e0-9c89-11e8-83c2-29858bdaf9cc.png> > > Regarding a dropdown, just FYI, @tynes <https://github.com/tynes> > recently added this: > https://github.com/bpanel-org/bpanel-ui/blob/master/src/components/Dropdown/Dropdown.js > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#15 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AR3NqKuOapi-8TLSlI5Zjfi30IDzCItnks5uPcS1gaJpZM4Vy_Xw> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJMktwMUuOvIAlw7E7Req7b72jyFdo3ks5uPcfOgaJpZM4Vy_Xw>.

xpranax added 2 commits August 7, 2018 15:23

Repair Radio Checking onChange and move Radio component to Types folder.

f056c69

optional starting checked prop

9840ed2

bucko13 reviewed Aug 8, 2018

View reviewed changes

improve option handling with tuple and string compatability ->

0bd3275

bucko13 reviewed Aug 9, 2018

View reviewed changes

bucko13 reviewed Aug 10, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repair Radio Checking onChange #15

Repair Radio Checking onChange #15

terrapanda commented Aug 7, 2018 •

edited

Loading

bucko13 Aug 8, 2018

bucko13 Aug 8, 2018

terrapanda commented Aug 8, 2018 via email

bucko13 commented Aug 8, 2018

terrapanda commented Aug 8, 2018

terrapanda commented Aug 8, 2018

bucko13 left a comment

bucko13 Aug 9, 2018

terrapanda Aug 10, 2018

tynes Aug 21, 2018

bucko13 Aug 9, 2018

terrapanda Aug 10, 2018

bucko13 Aug 10, 2018

terrapanda commented Aug 10, 2018

bucko13 commented Aug 10, 2018

bucko13 Aug 10, 2018

bucko13 Aug 10, 2018

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018 via email

Repair Radio Checking onChange #15

Are you sure you want to change the base?

Repair Radio Checking onChange #15

Conversation

terrapanda commented Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrapanda commented Aug 8, 2018 via email

bucko13 commented Aug 8, 2018

terrapanda commented Aug 8, 2018

terrapanda commented Aug 8, 2018

bucko13 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

terrapanda commented Aug 10, 2018

bucko13 commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018

terrapanda commented Aug 10, 2018 via email

bucko13 commented Aug 10, 2018 via email

terrapanda commented Aug 7, 2018 •

edited

Loading