-
Notifications
You must be signed in to change notification settings - Fork 577
feat: message hover and click behavior and modal #2352
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
feat: message hover and click behavior and modal #2352
Conversation
…nstead of getting from modal.js
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/dynamic-messaging #2352 +/- ##
============================================================
Coverage ? 52.79%
============================================================
Files ? 106
Lines ? 2146
Branches ? 643
============================================================
Hits ? 1133
Misses ? 910
Partials ? 103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
[FPTI_KEY.BUTTON_SESSION_UID]: buttonSessionID, | ||
}); | ||
|
||
const modalInstance = await getModal(clientID, merchantID); |
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 the error scenario inside getModal
that results in undefined
being returned here. We may want to include a check against that before calling modalInstance.show()
which would then throw another error.
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.
what do we do if getModal
is undefined, just not return anything?
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.
Yeah. Something as simple as return modalInstance?.show()
would work.
src/zoid/buttons/component.jsx
Outdated
creditProductIdentifier, | ||
}) => { | ||
const { message, buttonSessionID } = props; | ||
const amount = message?.amount; |
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.
Is this able to be directly included in the logger like the other fields instead of creating a variable that isn't really reused?
@wsbrunson we're ready for you to take another look at this PR, at your convenience |
Description
Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
Reproduction Steps (if applicable)
Screenshots (if applicable)
Dependent Changes (if applicable)
Groups who should review (if applicable)
❤️ Thank you!