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
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 thelabel
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 thelabel
prop 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 whetherdisabled
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?
- 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 thisouter
andinner
, and then changed them toradio
andradioButton
. I'm totally open to naming suggestions. - Right now there is a style prop for the
radioButton
calledbuttonStyle
. Should there also be a separate style prop for the outer Radio?
-
size
prop
- In the InAppMessaging demo, the
size
prop originally took anumber
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 stringssmall
andlarge
. 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.
⚠️ 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