-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(in-app-messaging): Add modal UI component #9712
feat(in-app-messaging): Add modal UI component #9712
Conversation
@Samaritan1011001 Thanks for opening this! Can you please add screenshots similar to the In-App Messaging |
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.
@Samaritan1011001 Looks like a great start! Left some minor feedback
packages/aws-amplify-react-native/src/InAppMessaging/components/constants.ts
Outdated
Show resolved
Hide resolved
...ify-react-native/src/InAppMessaging/components/hooks/useMessageProps/__tests__/utils.spec.ts
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/styles.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## in-app-messaging/staging #9712 +/- ##
============================================================
+ Coverage 74.52% 75.34% +0.82%
============================================================
Files 318 319 +1
Lines 20284 19750 -534
Branches 4421 4312 -109
============================================================
- Hits 15117 14881 -236
+ Misses 5028 4763 -265
+ Partials 139 106 -33
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
Left mostly styling related comments. Thanks for addressing the feedback!
packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/styles.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/styles.ts
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/styles.ts
Outdated
Show resolved
Hide resolved
imageContainer: { | ||
flex: 1, | ||
alignItems: 'center', | ||
marginVertical: SPACING_LARGE, |
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.
Wonder if we should remove this property so the top of the close button and the top of the image line up. Also, why do we need a flex
property here?
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.
Yeah, removed marginVertical
Flex is needed to center the image while keeping the x mark at the end of the row.
packages/aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/styles.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/ModalMessage.tsx
Outdated
Show resolved
Hide resolved
.../InAppMessaging/components/BannerMessage/__tests__/__snapshots__/BannerMessage.spec.tsx.snap
Outdated
Show resolved
Hide resolved
Hey @calebpollman, made the changes we discussed and the image is centered within the red box as shown below for reference. Let me know if that's what we had in mind. |
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 working through all the styling issues @Samaritan1011001 🚢
packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/styles.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
...ify-react-native/src/InAppMessaging/components/hooks/useMessageProps/__tests__/utils.spec.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/constants.ts
Outdated
Show resolved
Hide resolved
...aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/__tests__/Modal.spec.tsx
Outdated
Show resolved
Hide resolved
...aws-amplify-react-native/src/InAppMessaging/components/ModalMessage/__tests__/Modal.spec.tsx
Outdated
Show resolved
Hide resolved
...ify-react-native/src/InAppMessaging/components/hooks/useMessageProps/__tests__/utils.spec.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Caleb Pollman <cpollman1@gmail.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.
Lgtm thanks!
packages/aws-amplify-react-native/src/InAppMessaging/components/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Description of changes
Adds the modal support to in-app-messaging. Similar to a Full Screen message with a slightly different layout.
A function in
utils.ts
is modified to fit the needed design for modal.Tests have been modified and added to validated the changes.
PORTRAIT
LANDSCAPE
Issue #, if available
Description of how you validated changes
Different test cases as mentioned below were manually validated in addition to the unit tests for Modal.
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.