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

chore(ui-react-native): Migrate Checkbox primitive

Open ioanabrooks opened this issue 3 years ago • 6 comments

Description of changes

This change:

  • migrates the checkbox primitive from react-native example package;
  • adds a global storybook decorator and removes all occurences from primitives;
  • refactors accessibilityRole for checkbox, heading and label primitives to allow users to specify it, while still providing defaults.

Kapture 2022-09-22 at 11 17 25

Issue #, if available

Description of how you validated changes

Ran storybook and example app in iOS and android.

Checklist

  • [ ] 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.

ioanabrooks avatar Sep 20 '22 22:09 ioanabrooks

⚠️ No Changeset found

Latest commit: 42073f21ae80abe6bb106b1d162cdae25f315ab2

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 20 '22 22:09 changeset-bot[bot]

This pull request introduces 2 alerts when merging e5fa9564902ecf6cb9707e910d1b0884adb83182 into 3848ca89b686984ef571a441435d6dd7e4251139 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

lgtm-com[bot] avatar Sep 20 '22 22:09 lgtm-com[bot]

Nice job! Two global comments:

  1. Should this component be named Checkbox or CheckboxField? In the RWA, it's named CheckboxField, but there's also a Checkbox component under the hood.
  2. Should we add visual touch feedback based on platform expectations (e.g., material design touch feedback for Android, and iOS human interface guidelines). This could also include hitSlop handling for platform-specific touch target sizes
  1. It should be named Checkbox bc it's not a form component. CheckboxField has validations for form purposes.
  2. Yeah I can look into that now, honestly was thinking to defer styling for the theming work, since will have to take a second pass on this anyways.

ioanabrooks avatar Sep 21 '22 21:09 ioanabrooks

This pull request introduces 2 alerts when merging 5e7c98ee7e85c6b7d9d6cf1d0fe28541a8866ab7 into 3848ca89b686984ef571a441435d6dd7e4251139 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

lgtm-com[bot] avatar Sep 21 '22 22:09 lgtm-com[bot]

As per offline sync, to have parity with our react web component, th elabel should respond to press events as well as the icon. As follow-up PR, after the Icon primitive is implemented, the Checkbox primitive will be refactored to use a Pressable with Icon and Label.

ioanabrooks avatar Sep 22 '22 18:09 ioanabrooks

This pull request introduces 2 alerts when merging 151692d4a416118f62920eda5c36659839b94812 into 3848ca89b686984ef571a441435d6dd7e4251139 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

lgtm-com[bot] avatar Sep 22 '22 18:09 lgtm-com[bot]