-
Notifications
You must be signed in to change notification settings - Fork 38
Content card containers #540
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
base: staging
Are you sure you want to change the base?
Conversation
Add Image only template
remove changes in Contentview, will make the changes in the other PR.
remove extra line
Update Image only template and bug fixes
Update image only test
Fix Content Card View outdated ContentTemplates
…sForSurfaces([surface]) initialize page Update the failing unit test and updated sample app
Fix Content Card View outdated ContentTemplates
update integration with track PR
…e into content-card1
Upate RCTAEPMessaging
fix unit test
merge update
Synced with Staging branch and integrate with new Track API
Update tutorial doc for content card
update Content card customization guide
| return ( | ||
| <ContentCardContainerProvider settings={settings}> | ||
| <Text accessibilityRole="header" style={[styles.heading, { color: headingColor }]}>{heading.content}</Text> | ||
| <FlatList |
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 you confirm if this has been manually tested by rotating the device from portrait to landscape, especially for the horizontal FlatList case?
useWindowDimensions should recalculate the width automatically, but I just want to make sure none of the styles break when the orientation changes.
| surface, | ||
| style, | ||
| CardProps, | ||
| refetch, |
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.
Quick question: why didn’t we wire refetch to the FlatList onRefresh prop? Is there a reason we handled refresh differently?
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.
removed refetch logic from ContentCardContainer as it is not needed anymore
| <ContentCardView | ||
| template={item} | ||
| {...CardProps} | ||
| listener={(...args) => { |
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.
We should avoid creating a new inline listener inside renderItem for every item. This causes unnecessary re-renders and prevents ContentCardView from taking advantage of memoization.
If ContentCardView passes the card's id when firing the event (e.g., listener(event, item.id)), we can use a single shared callback:
const handleCardEvent = useCallback((event, itemId, ...rest) => {
if (event === "onDismiss") onDismiss(itemId);
CardProps?.listener?.(event, itemId, ...rest);
}, [onDismiss, CardProps]);<ContentCardView
template={item}
{...CardProps}
listener={handleCardEvent} // shared, stable listener
/>This avoids per-item function creation and improves FlatList performance.
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.
added the usage of a new single shared callback:
const handleCardEvent = useCallback(
(event?: ContentViewEvent, data?: ContentTemplate, nativeEvent?: any) => {
if (event === 'onDismiss' && data?.id) {
setDismissedIds((prev) => {
const next = new Set(prev);
next.add(data.id as any);
return next;
});
}
CardProps?.listener?.(event, data, nativeEvent);
},
[CardProps]
);
| image={emptyStateSettings?.image?.[colorScheme ?? "light"]?.url ?? ''} | ||
| text={ | ||
| emptyStateSettings?.message?.content || | ||
| "No Content Available" |
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.
Should localise it or use constants for rendering text
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.
added a constant for "No Content Available"
| const EmptyState: React.FC<EmptyStateProps> = ({ image, text }) => { | ||
| return ( | ||
| <CenteredView> | ||
| <Image source={{ uri: image }} style={{ width: 120, height: 120, padding: 10 }} resizeMode="contain"/> |
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 quick question: Should we add fallback image or not for network failure cases
| template.track?.("content_clicked", MessagingEdgeEventType.INTERACT, null); | ||
|
|
||
| // Mark as read when interacted with | ||
| template.isRead = true; |
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.
Mutating props directly is generally an anti-pattern. Instead of changing the prop object, can we handle this using internal useState or by emitting an event so the parent updates the list data?
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.
moved to the usage of an internal useState
|
|
||
| if (template.data?.content?.actionUrl) { | ||
| try { | ||
| Linking.openURL(template.data.content.actionUrl); |
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.
Use Linking.canOpenURL() and await Linking.openURL() with try/catch to avoid silent failures.
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.
Added Linking.canOpenURL() logic to ContentCardView.tsx for link validation
| setIsVisible(false); | ||
| }, [listener, template]); | ||
|
|
||
| const onPress = useCallback(() => { |
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 noticed that each <Button> receives the same onPress={onPress} handler, but onPress opens template.data.content.actionUrl.
Since each button has its own button.actionUrl, shouldn’t the tap action open the specific button’s URL instead of the card-level URL?
<Button key={button.id} title={button.text.content} onPress={onPress} // opens template.data.content.actionUrl ... />
Should this instead call a handler like handleButtonPress(button) so we correctly open button.actionUrl and track the right interaction?
Just checking the intended behaviour here — is it expected that all buttons open the same action URL, or should each button trigger its own?
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.
added onButtonPress for button specific behavior
|
|
||
| if (!template.data) return null; | ||
|
|
||
| const content = template?.data?.content as any; |
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 we define the types if possible?
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.
changed to const content = template?.data?.content as ContentCardContent;
| key={button.id} | ||
| actionUrl={button.actionUrl} | ||
| title={button.text.content} | ||
| onPress={onPress} |
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.
Noticed that all buttons are using the same onPress callback, so it’s unclear how we distinguish which button was clicked. Should we pass a button ID or some identifier to the handler, or is the current approach intentional?
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.
added logic for the interact id interactId={button.id}
| ]} | ||
| resizeMode="contain" | ||
| {...ImageProps} | ||
| /> |
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.
From a UI perspective, should we consider adding an image-loading indicator? This would give users feedback while the image is being fetched.
Something like the snippet below could help manage loading and error states:
const [imageError, setImageError] = useState(false);
const [imageLoading, setImageLoading] = useState(true);
<Image
source={{ uri: imageUri }}
onLoadStart={() => setImageLoading(true)}
onLoadEnd={() => setImageLoading(false)}
onError={() => {
setImageError(true);
setImageLoading(false);
}}
style={[
// ...styles
imageLoading && { opacity: 0.5 },
]}
{...ImageProps}
/>
{imageLoading && <ActivityIndicator />}
| imageContainer: { | ||
| alignItems: "center", | ||
| borderRadius: 12, | ||
| backgroundColor: "#f0f0f0", |
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.
Add it to color constant
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.
added imageContainerColor to theme colors interface
| style, | ||
| ...props | ||
| }: DismissButtonProps) => { | ||
| const colorScheme = useColorScheme(); |
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 few places we are using useTheme hooks but here we are using useColorScheme . Is it intentional?
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.
moved to usage of useTheme everywhere & removed useColorScheme
| textStyle, | ||
| ]} | ||
| > | ||
| x |
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.
Using lowercase "x" is not standard. Should use "×" (multiplication sign) or an icon.
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.
It might be better to use a cross icon instead of text here. Using text can shift or distort the button layout when system font settings change.
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.
Moved to usage of multiplication sign unicode
| position: "absolute", | ||
| top: 6, | ||
| right: 6, | ||
| zIndex: 1000, |
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 don’t see any recent (RN 0.70+) documentation or benchmarks that explicitly call out extremely high zIndex values (like 1000) as a performance bottleneck — but there are several past and known issues in React Native that make me cautious about using such high values:
- On Android, very large or dynamically changing
zIndexin aFlatListheader has been reported to cause hard crashes. ([Stack Overflow]1) zIndex+position: 'absolute'can behave inconsistently on Android unless component order andelevationare handled properly. ([Stack Overflow]2)- There was a long‑standing bug in older RN versions (v0.45–0.48) where Android would not re-render views correctly when
zIndexchanged dynamically. ([reubenab.github.io]3) - Multiple developers have reported unstable rendering (e.g. flickering or disappearing components) when relying heavily on
zIndex, advising use ofelevationon Android instead. ([Stack Overflow]4) - There are cases where
zIndexis “not taken seriously” on Android or gets overridden byelevation. ([Stack Overflow]5)
Recommendation:
Let’s reduce the zIndex to the minimum needed to achieve the stacking effect, instead of using a very large arbitrary number. On Android, we should prefer using elevation (or a combination of moderate zIndex + elevation) for more predictable behavior.
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.
removed zIndex: 1000 on DismissButton as stacking is handled by render order + absolute positioning
| { | ||
| backgroundColor: | ||
| colorScheme === "dark" | ||
| ? "rgba(255,255,255,0.1)" |
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.
Please add this to color constant
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.
moved to the usage of useTheme colors
| <Text | ||
| style={[ | ||
| styles.text, | ||
| { color: colorScheme === "dark" ? "white" : "black" }, |
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.
try to add color constants
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.
moved to the usage of useTheme colors
| import { | ||
| View, | ||
| StyleSheet, | ||
| useColorScheme, |
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.
We already have a useTheme hook implemented. To maintain consistency across the codebase, it would be better to use useTheme (or similar theme/color hooks) or useColorScheme. Whatever you feel fine..
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.
came to a decision to use useTheme to maintain consistency through the codebase
| const prevVisiblePagesRef = useRef<number[]>([]); | ||
|
|
||
| // Default colors based on theme | ||
| const defaultActiveColor = isDark ? "#fff" : "#0a7ea4"; |
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.
add color constants
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.
added activeColor to colors interface
|
|
||
| // Default colors based on theme | ||
| const defaultActiveColor = isDark ? "#fff" : "#0a7ea4"; | ||
| const defaultInactiveColor = isDark ? "#9BA1A6" : "#687076"; |
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.
add color constants
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.
added inactiveColor to colors interface
| useNativeDriver: true, | ||
| }), | ||
| ]).start(); | ||
| }, [isActive, scaleAnim, opacityAnim]); |
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.
Need to stop animations at the time of clean up
return () => {
// stop both if unmounted
scaleAnim.stopAnimation();
opacityAnim.stopAnimation();
};
```
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.
added stopAnimation at time of clean up to Pagination
| // Use default contrasting colors for visibility | ||
| // Note: unread_bg.clr is for the card background, not the dot | ||
| const dotColor = useMemo(() => | ||
| colorScheme === 'dark' ? '#FF6B6B' : '#FF4444', |
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.
Add color constants
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.
added dotColor to colors interface
| const styles = StyleSheet.create({ | ||
| container: { | ||
| position: 'absolute', | ||
| zIndex: 1000, |
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.
Please dont use such high value for zIndex
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.
removed zIndex: 1000 on UnreadIcon as stacking is handled by render order + absolute positioning
| View, | ||
| ViewProps, | ||
| ViewStyle, | ||
| useColorScheme |
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 we use useTheme hook or not?
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.
changed to usage of useTheme
| await Messaging.updatePropositionsForSurfaces([surface]); | ||
| const content = await Messaging.getContentCardUI(surface); | ||
| setContent(content); | ||
| setIsLoading(false); |
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.
remove this state as we are already setting it to false in finally black
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.
removed setIsLoading(false);
| setIsLoading(true); | ||
| const settings = await Messaging.getContentCardContainer(surface); | ||
| setSettings(settings); | ||
| setIsLoading(false); |
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.
remove this as we are already doing in finally block
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.
removed setIsLoading(false);
| if (context === undefined) { | ||
| return defaultTheme[systemColorScheme ?? 'light']; | ||
| } | ||
| return context; |
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 thinking the use case if someone want to toggle the themes in the content card itself. if so then we have to expose the setTheme function too
|
A couple of things appear to be missing from this PR: Accessibility support for the UI elements – we should ensure proper accessibility labels, roles, and hints are included. data-test-id attributes – are we planning to add UI test cases? If so, these identifiers will be needed to reliably target elements in tests. Adding these will help improve both accessibility and testability of the components. |
* adding empty state * addressing pr feed * addressing pr feedback * fixing empty state in android * adding use callback & moving styling * changing to inbox for default --------- Co-authored-by: Julia Kartasheva <[email protected]>
Background -
We're implementing the Messaging Inbox feature with support for:
1. Container Settings - The SDK reads container payload and displays content according to supported settings configured via AJO Authoring UI (experience.adobe.com).
2. Content Card Templates - Support for displaying content cards with various templates (SmallImage, LargeImage, ImageOnly). See our usage documentation for implementation details.
Key Changes -
Most of the additions are located in the packages/messaging/src/ui/ folder, which contains:
• React components for Content Card rendering (ContentCardContainer, ContentCardView…)
• Supporting UI components (Button, DismissButton, UnreadIcon, EmptyState)
• Hooks for data fetching (useContentCardUI, useContentContainer…)
• Theme system and styling
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: