feat(react): add useMessageProps hook for In-App Messaging #2340
Conversation
|
packages/react/src/components/InAppMessaging/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
| styleParams: { payloadStyle, overrideStyle }, | ||
| }); | ||
|
|
||
| expect(output).toMatchSnapshot(); |
There was a problem hiding this comment.
nit: rather than rely on comparing the snapshot to the test code and relying on humans not to just update the snapshot, what do you think of expecting it to match an expectedStyle object we can read right here. That would make me more confident in the test.
There was a problem hiding this comment.
I think part of why we just opted for snapshots here was it was already the case with the RN versions of these utils. Imho, we should lean on snapshot tests for a quick (and often large) tree output and trust that our contributors respect them and make updates only after they have validated the changes are intentional. Personally, I find that to be the exact same exercise as just updating some expectedStyle object but I do understand your concern. But I will add a task to re-evaluate this approach holistically and swap out snapshot tests all at once if the team is unconvinced 👍
| /** | ||
| * Resolved message types | ||
| */ | ||
| export interface MessageComponentStyles { |
There was a problem hiding this comment.
These are the styles provided from pinpoint, right? Should we add a comment describing why these styles are the only ones that can be overwritten?
There was a problem hiding this comment.
These aren't all coming from Pinpoint (e.g. image and close button styles aren't coming from payload). I think they represent (almost?) everything that is displayed in a message component, however, so we are actually allowing a lot of overriding via styles. Maybe the comment can be shored up (admittedly, it was copied from the React Native version of this hook)
| import { MessagePayloadStyle } from '@aws-amplify/ui-react-core'; | ||
| import React from 'react'; | ||
|
|
||
| // import { ImageDimensions } from '../useMessageImage'; |
| styles: MessageComponentStyles | null; | ||
| } | ||
|
|
||
| export interface MessageStyleParams { |
There was a problem hiding this comment.
Is this extra interface necessary, or could getMessageStyles just take a single argument of type StyleParams? I guess the question is, are you expecting to receive add more properties here?
There was a problem hiding this comment.
A large part of doing this was simply to maintain parity with the React Native counterpart. We could scrap it and just inline the StyleParams typing in the function arguments but it is probably cleaner and more easily extensible (should we decide we do need to add more arguments especially as we add more complex components e.g. Carousel - even though I don't anticipate it yet).
…rops/utils.ts Co-authored-by: Scott Rees <[email protected]>
Ideally, probably not. But we know that the PR for that is in flight also and this code needs to be added anyway. There's no real reason this commit would be all that useful on its own. |
…shfang/amplify-ui into in-app-messaging/add-use-message-props
| import { MessagePayloadStyle } from '@aws-amplify/ui-react-core'; | ||
| import React from 'react'; | ||
|
|
||
| // import { ImageDimensions } from '../useMessageImage'; |
There was a problem hiding this comment.
Think we can delete this line, web useMessageImage does not return ImageDimensions
packages/react/src/components/InAppMessaging/hooks/useMessageProps/utils.ts
Outdated
Show resolved
Hide resolved
…rops/utils.ts Co-authored-by: Caleb Pollman <[email protected]>
This PR adds a hook for use with In-App Messaging components (to be added in future PRs)
Description of how you validated changes
Tested with sample app and
yarn testChecklist
yarn testpassessideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.