chore(in-app-message): Normalize in-app message schema #1
Conversation
| ); | ||
| }); | ||
| } | ||
| return this.normalizeMessages( |
There was a problem hiding this comment.
What do you think of normalizing the response before retuning the messages in getInAppMessages?
There was a problem hiding this comment.
I think there could be benefits to storing the messages as the provider has it - and also not sure about serializing when the messages have handlers attached. If anything, I'm wondering if we even need to return anything with the sync - maybe it should just be a Promise<void>
There was a problem hiding this comment.
not serializing the handlers makes sense. FYI I would be fine with the sync returning nothing.
| private normalizeMessages = ( | ||
| messages: PinpointInAppMessage[] | ||
| ): InAppMessage[] => { | ||
| const that = this; |
There was a problem hiding this comment.
Just curious, what is the reasoning behind declaring that?
There was a problem hiding this comment.
Inside the scope of the newly created objects, this becomes that object itself, but we want to reference the outer this which is the class instance.
packages/notifications/src/types.ts
Outdated
| title?: string; | ||
| action?: InAppMessageAction; |
There was a problem hiding this comment.
How do you fell abt updating these to required props and swapping title for content?
There was a problem hiding this comment.
We can make it required, sure. Not sure about the title for content given that it's a button. With header and body, we kind of know there's intent for potentially rich text content. Do we envision a reason that there'd ever be anything other than the text that's intended for the button here?
There was a problem hiding this comment.
Potential for a component to be passed as title? I agree with what your are saying, but i also see an argument for making the prop a bit more generic
There was a problem hiding this comment.
Isn't this a string coming from the console?
There was a problem hiding this comment.
In the case of Pinpoint yes, but in the case of other implementations maybe not?
There was a problem hiding this comment.
Although this whole conversation implies that string as a type is not going to work, which i do disagree with. Fine with just keeping it as title unless you feel differently
… message which is only meant to be stored
|
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
This PR updates the category in-app message schema to be non-provider specific.
Description of how you validated changes
Manually tested with external React Native app
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.