-
Notifications
You must be signed in to change notification settings - Fork 38
Add unread UI implementation #539
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
Conversation
Add unread UI implementation
update the unread icon mock example
Use useContainerSettigs hook and update the dist files
update unreadIcon for error handling
packages/messaging/src/ui/components/ContentCardView/ContentCardView.tsx
Show resolved
Hide resolved
| : styles.container, | ||
| styleOverrides?.container | ||
| styleOverrides?.container, | ||
| getUnreadBackgroundColor() && { backgroundColor: getUnreadBackgroundColor() } |
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.
These functions cannot be memoized by react, so Id either memoize them or just handle them inline, which reads much cleaner
| } | ||
|
|
||
| // Helper function to convert placement from settings to component position | ||
| const convertPlacement = (placement: 'topleft' | 'topright' | 'bottomleft' | 'bottomright'): 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right' => { |
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.
not sure we really need this, also Id create a type for the placement if you want to keep it
|
|
||
| switch (displayPosition) { | ||
| case 'top-left': | ||
| return { ...baseStyle, top: 6, left: 6 }; |
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.
since baseStyle is applied everywhere, you can just keep it in the styles of the component its applied to, Id also just add these styles in StyleSheet.create so they can be optimized
| } | ||
| }; | ||
|
|
||
| const getDotColor = () => { |
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.
this doesnt need to be a function, you can just do
const dotColor = useMemo(() => colorScheme === 'dark'...
| const unreadSettings = settings.content.unread_indicator; | ||
|
|
||
| // Use settings from context with fallbacks to props | ||
| const displaySize = size; |
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.
This just reassigns the variable and then it isnt really used differently?
| } | ||
|
|
||
| // If image failed to load, fallback to dot | ||
| if (renderType === 'image' && imageLoadError) { |
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 logic 👍
| // If image failed to load, fallback to dot | ||
| if (renderType === 'image' && imageLoadError) { | ||
| return ( | ||
| <View |
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.
But here i'd make this a separate component since its the same as below.
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.
Maybe <Dot /> ?
| } | ||
|
|
||
| if (renderType === 'image' && (imageSource || darkImageSource)) { | ||
| const finalImageSource = colorScheme === 'dark' && darkImageSource ? darkImageSource : imageSource; |
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.
id memoize this above
6997afe to
81521eb
Compare
Update with review comments
81521eb to
00e7269
Compare
…ct-native into unreadIcon
fix the building errors for sample app and update the sample app
| const containerSettings = useContainerSettings(); | ||
|
|
||
| // Support both controlled and uncontrolled modes | ||
| const isRead = isReadProp !== undefined ? isReadProp : internalIsRead; |
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.
Do we want to support isRead via prop? Or should it be a container settings prop? Something like enableUnread which defaults to true? I don't have strong opinions either way
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 made it to use the container settings Prop to match what Android is doing.
| @@ -0,0 +1,325 @@ | |||
| // /* | |||
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.
This test should be able to be re-enabled with the latest updates
| type?: 'dot' | 'image'; | ||
| } | ||
|
|
||
| const Dot = ({ size, backgroundColor }: { size: number; backgroundColor: string }) => ( |
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 make an interface above for this?
| const darkImageSource = unreadSettings?.unread_icon?.image?.darkUrl ? | ||
| { uri: unreadSettings.unread_icon.image.darkUrl } : darkSource; | ||
|
|
||
| const getPositionStyle = () => { |
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.
instead of a function, you can do a useMemo to return the styles, and then apply it below. Optimizes it a tiny bit :)
|
|
||
| const renderContent = () => { | ||
| // Check if we should show dot instead of image based on URL availability | ||
| const shouldShowDot = |
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 for dark mode we might also want to check if the default image url is empty?
| } | ||
|
|
||
| // If image failed to load, fallback to dot | ||
| if (renderType === 'image' && imageLoadError) { |
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.
❤️ love this
| getPositionStyle(), | ||
| { minWidth: size, minHeight: size }, | ||
| containerStyle, | ||
| typeof style === 'object' ? style : undefined |
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 here you can just pass style and not check its type. If its an invalid value it should be stripped.
| [colorScheme, darkImageSource, imageSource] | ||
| ); | ||
|
|
||
| const renderContent = () => { |
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.
if youd like to do this as a function, id wrap it in a useCallback
update with review comments
| @@ -1,4 +1,5 @@ | |||
| import React, { createContext } from "react"; | |||
| import { SettingsPlacement } from "../components/UnreadIcon/UnreadIcon"; | |||
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'd probably invert these, put the SettingsPlacement in this file and consume it inside the component.
| } | ||
| export class ContentCard extends PropositionItem { | ||
| data: ContentCardData['data']; | ||
| read: boolean = 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.
lets name this isRead
| {...DismissButtonProps} | ||
| /> | ||
| )} | ||
| {(containerSettings?.content?.isUnreadEnabled ?? true) && !isRead && ( |
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.
you can use isUnreadEnabled since its the same logic
| // Mark as read when interacted with (matches Android behavior) | ||
| template.read = true; | ||
| // Trigger re-render to update unread indicator | ||
| forceUpdate({}); |
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.
hmmm this likely means something isnt being reflected in the state probably, we should look for a fix.
update for the new comments
Update test
Update metro.config and dist files
Update few UI color in the sample app
update the code to automatically refetch the content card
This reverts commit f8e1f1a.
update read status from native
packages/messaging/src/Messaging.ts
Outdated
| return new ContentTemplate(card, type); | ||
| const read = card.data?.read; | ||
| const contentTemplate = new ContentTemplate(card, type); | ||
| contentTemplate.isRead = read ?? 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.
you probably want to take this as a parameter inside the constructor for contentTemplate
Add isRead to contentTemplate constructor
Add unread UI implementation
Update the dist files, so a lot of files are changed. You can ignore the dist files.
Description
Example:
Light mode with image icon set - use image and unread background color

Dark mode with no image url for icon - default to dot and unread background color

Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: