[Android] "What’s New" promo message: Add new message type to show a list view #7072
Conversation
… RMF called What's New. This message type uses a cards_list content structure, allowing for a title, description, and a list of items, each with its own title, description, image, and action.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
anikiki
changed the title
…ge_add_new_message_type_to_show_a_list_view
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Few comments that I think is best if we review them together.
code looks great, it's just to discuss merging strategy around RMF.
| enum class MessageType { | ||
| REMOTE_MESSAGE, | ||
| REMOTE_PROMO_MESSAGE, | ||
| REMOTE_LIST_MESSAGE, |
There was a problem hiding this comment.
Do we want to define this type here? The new message is not supported as MessageCta, I'd expect this class doesn't change.
| when (message.messageType) { | ||
| REMOTE_MESSAGE -> setRemoteMessage(message) | ||
| REMOTE_PROMO_MESSAGE -> setPromoMessage(message) | ||
| REMOTE_LIST_MESSAGE -> { } |
There was a problem hiding this comment.
if we drop the enum value, we can remove.
| promoAction = content.actionText, | ||
| messageType = MessageType.REMOTE_PROMO_MESSAGE, | ||
| ) | ||
| is CardsList -> Message( |
There was a problem hiding this comment.
The mapper here is used to receive a remote message and map it into Message so it's rendered as MessageCTA in the NTP.
However the new type, it's not expected to be a MessageCTA in NTP, so we should not map it here.
Did I miss something and we want to show it somehow in NTP as a normal card?
Let me know if you want to catch up on this. I'd like to know how you are planning to show the modal, that's the place where I would expect we have a similar mapping.
| } else { | ||
| R.drawable.ic_visual_design_update_artwork_dark | ||
| } | ||
| IMAGE_AI -> R.drawable.ic_image_ai |
| promoAction = content.actionText, | ||
| messageType = MessageType.REMOTE_PROMO_MESSAGE, | ||
| ) | ||
| is CardsList -> Message( |
| context.startActivity(browserNav.openInCurrentTab(context, url)) | ||
| } | ||
|
|
||
| private fun submitUrlInContext(url: String) { |
There was a problem hiding this comment.
can you leave a TODO if we plan to cover this in another PR?
| context.startActivity(browserNav.openInCurrentTab(context, url)) | ||
| } | ||
|
|
||
| private fun submitUrlInContext(url: String) { |
There was a problem hiding this comment.
Can you leave a TODO if we plan to cover this in another PR?
| @ContributesMultibinding( | ||
| AppScope::class, | ||
| ) | ||
| class UrlInContextActionMapper @Inject constructor() : MessageActionMapperPlugin { |
There was a problem hiding this comment.
Actions are generic types that will be available to all remote messages.
If we add this now, we add support to this action on any remote message, not only cards, however the implementation is not completed (we don't handle the event in the UI).
So we will have few app versions out there, that know how to process this action from the config, but have an empty implementation.
I'd say is preferred if we implement and merge the action when it's complete.
If you prefer not to do it now, but keep the code:
- remove the DI annotation so you keep the code, but it's not used until you implement it entirely, then inject it
or 2) introduce a FF or some value to make it disabled
for simplicity I would go for option 1.
There was a problem hiding this comment.
"Similar" comment to the one I left on the Action mapping.
Right now, we are telling RMF how to process the new type, so after merging, clients will now how to process a config with the new message type. Which means that for this app versions onward, the message is valid.
It's great you already thought about this, and in the UI, we won't show it until is fully supported.
However, it's better to treat it as invalid until it's fully supported.
Quick explanation why, but probably best over zoom:
- app downloads the config
- the process the config and they will show first valid message the user is eligible, stops processing the rest
- when the user interacts or dismisses that message, next time we process the config other messages will be eligible
- if an app version doesn't recognises a message/action/matchingattribute, it skips the message and continues processing other messages
I believe by introducing "how to process" but not "how to render", we are creating an unexpected scenario. We will process message, it will be valid, we won't process other messages but at the same time it won't appear in the UI.
Happy to chat more about this. But overall I think, if we want to start merging this logic, we should FF or similar, so we can enable the logic starting with minAppVersion.
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1211783737656574?focus=true
Description
Added support for a new "What's New" message type in the remote messaging system. This includes:
REMOTE_WHATS_NEW_MESSAGEtype to theMessageCtaclass.CardsListcontent type for displaying feature cards.UrlInContextaction type to open URLs in a contextual webview.Steps to test this PR
See Asana for more details.
NO UI changes