chore(in-app-messaging): add in-app messaging component and context unit tests #9551
Conversation
calebpollman
requested review from
cshfang,
reesscot and
elorzafe
and removed request for
Ashish-Nanda
Codecov Report
@@ Coverage Diff @@
## in-app-messaging/staging #9551 +/- ##
============================================================
+ Coverage 74.36% 74.70% +0.34%
============================================================
Files 312 312
Lines 20219 20223 +4
Branches 4406 4410 +4
============================================================
+ Hits 15036 15108 +72
+ Misses 5044 4977 -67
+ Partials 139 138 -1
Continue to review full report at Codecov.
|
| @@ -19,7 +19,7 @@ import { | |||
| } from '../types'; | |||
|
|
|||
| export interface BannerMessageProps extends InAppMessageComponentBaseProps { | |||
| position: InAppMessageComponentPosition; | |||
| position?: InAppMessageComponentPosition; | |||
There was a problem hiding this comment.
What would happen if position is missing? Probably is protected wherever it is used. Just to check after
There was a problem hiding this comment.
👍🏽 There is a default value in the BannerMessage component
| > | ||
| {children} | ||
| </InAppMessagingContext.Provider> | ||
| const value = useMemo( |
There was a problem hiding this comment.
Did you see an improvement in terms of performance? How did you measure it?
There was a problem hiding this comment.
After looking in to perf again, not sure we need it at this time. Will remove for now
| <Text | ||
| style={ | ||
| Array [ | ||
| Object { | ||
| "alignSelf": "center", | ||
| }, | ||
| undefined, | ||
| ] |
There was a problem hiding this comment.
This is the default child component when passing a String as child?
There was a problem hiding this comment.
Yes - React Native will crash on a string pass a child that is not wrapped inside a Text component. Button has a conditional checking for whether the child is a string, and wrapping with a center aligned Text if not
| TestRenderer.act(() => { | ||
| renderer = TestRenderer.create( | ||
| <InAppMessagingProvider> | ||
| <TestComponent /> | ||
| </InAppMessagingProvider> | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is a little odd, I didnt get completely how all tests are using this renderervariable
There was a problem hiding this comment.
Can see that. If you feel strongly will move inside each it block
|
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
Issue #, if available
NA
Description of how you validated changes
yarn testChecklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.