amplify-ui
amplify-ui copied to clipboard
feat(ui-react-native): Radio
Description of changes
I have a number of open questions regarding the implementation for the Radio primitive:
- API for
labelprop
- 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
childrenrather than setting thelabelprop explicitly (see here). Should we maintain parity with RWA in this regard? - Also, in the RWA,
labelis an accessibility prop, while in this PR it is currently the visible label for the Radio button. I could use some clarity on what thelabelprop should be for RNA, and if we need a separate prop for accessibility.
- 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 whetherdisabledis 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
getStylesmethod that handles conditional styling?
- 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 thisouterandinner, and then changed them toradioandradioButton. I'm totally open to naming suggestions. - Right now there is a style prop for the
radioButtoncalledbuttonStyle. Should there also be a separate style prop for the outer Radio?
sizeprop
- In the InAppMessaging demo, the
sizeprop originally took anumbersince it just had to adjust the size of an icon. - In this PR, in an effort to maintain parity with RWA, the
sizeprop takes the stringssmallandlarge. 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 testpasses - [ ] Tests are updated
- [ ] No side effects or
sideEffectsfield 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.
⚠️ 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
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!
Please select the "squash and merge" option when merging in the future