Modernize polaris-radio-button component #504
Conversation
tomnez
force-pushed
the
refactor/es6-polaris-radio-button-DEV-76
branch
from
a085d4d to
3646e7c
Compare
| checked={{@checked}} | ||
| disabled={{@disabled}} | ||
| aria-describedby={{this.describedBy}} | ||
| onchange={{action (or @onChange this.onChange) value="target.value"}} |
There was a problem hiding this comment.
Left the action helper here because the fn wasn't working as expected when passing value="target.value" 🤷♂
There was a problem hiding this comment.
@tomnez can you try
<input ...... {{on "change" (or @onChange this.onChange)}} ....>
There was a problem hiding this comment.
This still passes the event as the first (and only) argument now rather than the expected value 🤷♂
There was a problem hiding this comment.
not really a problem, we can just handle in the local action right? as the example here
There was a problem hiding this comment.
Yeah we can do that 👍
| checked={{@checked}} | ||
| disabled={{@disabled}} | ||
| aria-describedby={{this.describedBy}} | ||
| onchange={{action (or @onChange this.onChange) value="target.value"}} |
There was a problem hiding this comment.
@tomnez can you try
<input ...... {{on "change" (or @onChange this.onChange)}} ....>
| disabled={{@disabled}} | ||
| aria-describedby={{this.describedBy}} | ||
| onchange={{action (or @onChange this.onChange) value="target.value"}} | ||
| onfocus={{fn (or @onFocus this.onFocus)}} |
There was a problem hiding this comment.
same...let's use the on modifier for these last 2
| checked={{@checked}} | ||
| disabled={{@disabled}} | ||
| aria-describedby={{this.describedBy}} | ||
| onchange={{action (or @onChange this.onChange) value="target.value"}} |
There was a problem hiding this comment.
not really a problem, we can just handle in the local action right? as the example here
| checked={{@checked}} | ||
| disabled={{@disabled}} | ||
| aria-describedby={{this.describedBy}} | ||
| {{on "change" (or @onChange this.onChange)}} |
There was a problem hiding this comment.
I think we still want to pass the value to the onChange action....assuming we were correct and that's what the react implementation does
PS: we shouldn't modify tests to match our implementation.....but make sure tests are testing correct behavior and we pass 😬
There was a problem hiding this comment.
check the example I've posted earlier....that should be in the component not in the tests
let me know if this doesn't makes sense pls
There was a problem hiding this comment.
Ah so you mean we want a middle-man method (something like a private handleChange action) where we grab the value and then call onChange?
Originally when you said
we can just handle in the local action
I thought you meant that the passed-in onChange would be responsible for this. Sorry about the confusion 😬
There was a problem hiding this comment.
yeah, like handleChange
moving this to the caller onChange would mean we break the API of this component...which we don't want
DEV-76
Updates
PolarisRadioButtonto tagless component, ES6 classes, and angle bracket syntax.Dummy app: