-
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 FullScreenMessage component #9130
feat(in-app-messaging): add FullScreenMessage component #9130
Conversation
// due to limitations with passing spacing style to the underlying style prop of the SafeAreaView of the | ||
// MessageWrapper, only pass backgroundColor as a message or override style, apply all other override style | ||
// values to container View | ||
const { containerStyle, wrapperStyle } = useMemo(() => { | ||
const { container: baseOverrideContainerStyle } = style ?? {}; | ||
const { backgroundColor, ...overrideContainerStyle } = Array.isArray(baseOverrideContainerStyle) | ||
? StyleSheet.flatten(baseOverrideContainerStyle) | ||
: (baseOverrideContainerStyle as ViewStyle) ?? {}; | ||
|
||
return { | ||
containerStyle: [styles.container, overrideContainerStyle], | ||
|
||
// prevent passing object with backgroundColor key and undefined or null value | ||
// to avoid overriding payload backgroundColor | ||
wrapperStyle: [container?.style, backgroundColor ? { backgroundColor } : null], | ||
}; | ||
}, [container?.style, style]); |
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.
Will be abstracting this in to a util in a follow up PR as this handling is needed for the Modal
and Carousel
components as well
width = maxImageDimension; | ||
} | ||
|
||
return StyleSheet.create({ imageStyle: { height, resizeMode: 'contain', width } }); |
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.
Landscape orientation images sizing in FullScreenMessage
components is slightly off, will be addressing in a follow up hook/util refactor
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.
I think there is a bug for isLandscape
, also it might be an issue if the image url is broken?
...s-amplify-react-native/src/InAppMessaging/hooks/useInAppMessageImage/useInAppMessageImage.ts
Show resolved
Hide resolved
...s-amplify-react-native/src/InAppMessaging/hooks/useInAppMessageImage/useInAppMessageImage.ts
Show resolved
Hide resolved
|
||
const isSquare = aspectRatio === 1; | ||
const isPortrait = imageHeight > imageWidth; | ||
const isLandcape = imageWidth < imageHeight; |
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.
I think this is incorrect, is the same as isPortrait
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.
Good catch :) Also misspelled 😱
Codecov Report
@@ Coverage Diff @@
## in-app-messaging/main #9130 +/- ##
======================================================
Coverage 78.00% 78.00%
======================================================
Files 250 250
Lines 18115 18115
Branches 3885 3885
======================================================
Hits 14131 14131
Misses 3854 3854
Partials 130 130 Continue to review full report at Codecov.
|
...s-amplify-react-native/src/InAppMessaging/components/FullScreenMessage/FullScreenMessage.tsx
Outdated
Show resolved
Hide resolved
...s-amplify-react-native/src/InAppMessaging/components/FullScreenMessage/FullScreenMessage.tsx
Show resolved
Hide resolved
{body?.content && <Text style={[styles.message, body.style, style?.message]}>{body.content}</Text>} | ||
</View> | ||
</View> | ||
{(hasPrimaryButton || hasSecondaryButton) && ( |
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.
Can this stuff be shared between message components somehow to be more DRY? Or is there a compelling reason not to?
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.
Will definitely move some of the variables in to a util in a follow up
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.
In terms of not making things more DRY immediately, have been treating these components in a nuanced manner to avoid creating utils that may not end up fitting in the future
...s-amplify-react-native/src/InAppMessaging/hooks/useInAppMessageImage/useInAppMessageImage.ts
Outdated
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.
LGTM! 🌮
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.
I'm still not sure avoiding a call to StyleSheet.flatten is worth checking if style is an array and making that bit of code a little harder to parse. But it is not a blocker either.
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
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
Add In-App Messaging
FullScreenMessage
React Native UI component for rendering InAppMessage payloads created in the Amazon Pinpoint console:Examples
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.