-
Notifications
You must be signed in to change notification settings - Fork 106
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
[#173171577] Extract front-matter CTA data from message content #1936
[#173171577] Extract front-matter CTA data from message content #1936
Conversation
add some utils function to: extra cta, check cta action, remove front-matter from message content
add type definition
Affected stories
New dependencies added: front-matterAuthor: Jason Campbell Description: Extract YAML front matter from a string Homepage: https://github.com/jxson/front-matter
|
ts/utils/messages.ts
Outdated
): Option<CTAS> => { | ||
return fromPredicate((t: string) => FM.test(t))(message.content.markdown) | ||
.map(m => FM<MessageCTA>(m).attributes) | ||
.chain(attrs => fromNullable(attrs[locale])); |
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 case we are working on a locale that is missing from the CTA we received in the message we return a none
and we won't show the cta even if it has been added to the message.
E.g. User changed to "EN" from preferences option in the message we received just the CTA on IT language -> no CTA displayed. Should we fallback on the received CTA?
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.
@CrisTofani
Yes it could happen.
What if we make it/en as required and not optional?
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.
@Undermaken
I think it could fail anyway cause we don't have the complete control of what we get since it is a remote content, maybe we should handle attrs
as an array of string and fallback on the first we get?
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.
@CrisTofani
starting from your warning I improved the logic and I added a test ae77f58:
now if the front-matter doesn't respect MessageCTA type the decoding will fail.
So if, from remote, we receive a data that doesn't contain it nor en the CTA won't be decoded.
Btw about the fallback we can do as follow: since at the moment the MessageCTA requires it/en, if the CTA for the current locale is not available we can fallback to the other one. if from remote we receive not it/en, fr for example, the decoding will fail
What do you think?
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.
Yep! I find it is a good point!
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.
update within 0dd7a0f
Short description:
This PR extracts cta data from message content (front-matter)