amplify-ui icon indicating copy to clipboard operation
amplify-ui copied to clipboard

feat(ui-react-native): Radio

Open joebuono opened this issue 2 years ago • 2 comments

Description of changes

I have a number of open questions regarding the implementation for the Radio primitive:

  1. API for label prop
  • Current API (migrated from InAppMessaging demo):
<Radio value="This is the value" label="This is the label" />
  • RWA API:
<Radio value="This is the value">This is the label</Radio>
  • The approach we took for the RWA was to pass the label as children rather than setting the label prop explicitly (see here). Should we maintain parity with RWA in this regard?
  • Also, in the RWA, label is an accessibility prop, while in this PR it is currently the visible label for the Radio button. I could use some clarity on what the label prop should be for RNA, and if we need a separate prop for accessibility.
  1. Handling styles for state
  • I was not certain about how to conditionally add styles based on state.
  • For example, this is how I styled the outer <View /> depending on whether disabled is true:
    <View
      {...rest}
      style={[
        styles.container,
        containerDirection,
        ...(disabled ? [styles._disabled] : []),
        style,
      ]}
    >

(I also did the same pattern for the size prop)

  • This looks funky to me, so I'm wondering if there's a better way to do it. Should there be a separate getStyles method that handles conditional styling?
  1. Nested styles
  • In the InAppMessaging demo, the Radio displayed a png in the <IconButton />.
  • In this PR, the icon is replaced with styled <View />s (one for outer circle, one for "dot"). I originally named this outer and inner, and then changed them to radio and radioButton. I'm totally open to naming suggestions.
  • Right now there is a style prop for the radioButton called buttonStyle. Should there also be a separate style prop for the outer Radio?
  1. size prop
  • In the InAppMessaging demo, the size prop originally took a number since it just had to adjust the size of an icon.
  • In this PR, in an effort to maintain parity with RWA, the size prop takes the strings small and large. since there are now two <View />s comprising the Radio. This adds some complexity, so I'm wondering if there's a better way to implement this.

https://user-images.githubusercontent.com/48109584/191571059-2ac102a8-f6eb-4eff-8876-b98c265ee893.mov

https://user-images.githubusercontent.com/48109584/191571083-d4044ead-087f-4481-8200-137939b5be5d.mov

Issue #, if available

Description of how you validated changes

Checklist

  • [X] PR description included
  • [ ] yarn test passes
  • [ ] Tests are updated
  • [ ] No side effects or sideEffects field updated
  • [ ] Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

joebuono avatar Sep 21 '22 17:09 joebuono

⚠️ No Changeset found

Latest commit: 8c2dce1caaa35bfbdbd20b2fdae35bbffd467edb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Sep 21 '22 17:09 changeset-bot[bot]

Leaving a general note, I see the demo videos posted are iOS only, want to make sure that this is tested in android as well, to help uncover any potential issues early. By running yarn run react-native-example storybook in one terminal and yarn run react-native-example dev in a second one, you can have both simulators open and control stories through the storybook UI.

For styling related questions, I would defer to later when we have theming in place, we will go back and change styling related things when adding theming. But this is a great start!

ioanabrooks avatar Sep 21 '22 17:09 ioanabrooks

Please select the "squash and merge" option when merging in the future

calebpollman avatar Sep 29 '22 23:09 calebpollman