-
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
chore(in-app-messaging): refactor InAppMessaging UI components to handle landscape mode #9897
chore(in-app-messaging): refactor InAppMessaging UI components to handle landscape mode #9897
Conversation
…dle landscape mode
...s-amplify-react-native/src/InAppMessaging/components/CarouselMessage/CarouselMessageItem.tsx
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/CarouselMessage/styles.ts
Outdated
Show resolved
Hide resolved
packages/aws-amplify-react-native/src/InAppMessaging/components/MessageLayout/types.ts
Show resolved
Hide resolved
...ages/aws-amplify-react-native/src/InAppMessaging/components/MessageLayout/PortraitLayout.tsx
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
Codecov Report
@@ Coverage Diff @@
## in-app-messaging/staging #9897 +/- ##
=========================================================
Coverage 79.27% 79.27%
=========================================================
Files 327 327
Lines 20633 20640 +7
Branches 4518 4506 -12
=========================================================
+ Hits 16357 16363 +6
- Misses 4170 4171 +1
Partials 106 106
📣 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.
Just a few minor comments/questions
packages/aws-amplify-react-native/src/InAppMessaging/components/MessageLayout/MessageLayout.tsx
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
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.
Approving since remaining feedback is just minor comment tweaks. Lgtm!
@@ -48,63 +48,87 @@ export const defaultStyle: CarouselMessageComponentStyle = StyleSheet.create({ | |||
}, | |||
}); | |||
|
|||
export const getStyles = (imageDimensions: ImageStyle): CarouselMessageStyle => | |||
const commonStyles: Omit<CarouselMessageStyle, 'image'> = { |
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 understand these commonStyles
are specific common styles for each of the message type but is there any benefit in having a common style across the message type? (modal, full screen, banner, carousel)
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.
That's a valid question. At this point we could definitely just share the button and text styles across all the Message components as they do not change based on layout or orientation, but with the size of this refactor being what it is decided to leave that alone for the time being
@@ -1,4 +1,4 @@ | |||
export { default as useMessage } from './useMessage'; | |||
export { default as useMessageImage, ImageDimensions } from './useMessageImage'; | |||
export { default as useMessageProps } from './useMessageProps'; | |||
export { default as useMessageProps, UseMessageProps } from './useMessageProps'; |
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.
Question: Why are there two similar named exports?
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.
UseMessageProps
is the return type of useMessageProps
@@ -30,7 +35,7 @@ export default function CarouselMessageItem(props: CarouselMessageItemProps) { | |||
|
|||
return ( | |||
<SafeAreaView style={styles.componentWrapper}> | |||
<FullScreenContent {...props} {...messageProps} /> | |||
<MessageLayout {...props} {...messageProps} orientation={deviceOrientation} /> |
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.
Curious: Where in code is the layout
being set so this common element can conditionally switch to different layouts?
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.
layout
is included in props
testID={IN_APP_MESSAGING.CLOSE_BUTTON} | ||
/> | ||
); | ||
|
||
return ( | ||
<View style={styles.container}> |
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.
Question: Does this generic MessageLayout
component handle the differences in layouts? I remember modal having slightly different layout than fullscreen and likewise with banner.
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.
After some discussion it was decided that we could closer align the component composition and would just leverage styling differences per orientation/layout
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.
Looks good. Just a couple of questions out of curiosity. Thank you!
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
Improve InAppMessaging UI components device orientation handling:
MessageLayout
,LandscapeLayout
, andPortraitLayout
componentsMessageLayout
getPortraitStyles
andgetLandscapeStyles
utils in UI components for resolving orientation specific stylingIssue #, if available
NA
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.