-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(ui-react-native): Migrate Checkbox primitive #2621
chore(ui-react-native): Migrate Checkbox primitive #2621
Conversation
|
This pull request introduces 2 alerts when merging e5fa956 into 3848ca8 - view on LGTM.com new alerts:
|
packages/react-native/src/primitives/Checkbox/__tests__/Checkbox.spec.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Two global comments:
-
Should this component be named
Checkbox
orCheckboxField
? In the RWA, it's namedCheckboxField
, but there's also aCheckbox
component under the hood. -
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Let's sync offline about the handling of selected
and value
, not sure if my initial POC was well thought through in that terms of how those props should interact and the event cycle around them
examples/react-native/src/features/InAppMessaging/Demo/Demo.tsx
Outdated
Show resolved
Hide resolved
This pull request introduces 2 alerts when merging 5e7c98e into 3848ca8 - view on LGTM.com new alerts:
|
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. |
This pull request introduces 2 alerts when merging 151692d into 3848ca8 - view on LGTM.com new alerts:
|
Co-authored-by: Caleb Pollman <cpollman@amazon.com>
onPress={handleOnChange} | ||
source={selected ? icons.checkboxFilled : icons.checkboxOutline} | ||
disabled | ||
source={checked ? icons.checkboxFilled : icons.checkboxOutline} | ||
size={size} | ||
style={iconButtonStyle} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure we clean up the buttonStyle
prop and iconButtonStyle
when we replace the IconButton
Co-authored-by: Caleb Pollman <cpollman@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this @ioanabrooks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Description of changes
This change:
Issue #, if available
Description of how you validated changes
Ran storybook and example app in iOS and android.
Checklist
yarn test
passessideEffects
field updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.