-
Notifications
You must be signed in to change notification settings - Fork 60
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: adds prop update listening to modal browser zoid polyfill #1161
base: develop
Are you sure you want to change the base?
Conversation
48ed5aa
to
ecf1a92
Compare
const merchantOrigin = decodeURIComponent(initialProps.origin); | ||
|
||
if (eventOrigin === merchantOrigin && eventName === 'PROPS_UPDATE' && newProps && typeof newProps === 'object') { | ||
// send event ack so PostMessenger will stop reposting event | ||
sendEventAck(id, merchantOrigin); |
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.
I know we chatted about this internally, but I think we can go with
const merchantOrigin = decodeURIComponent(initialProps.origin); | |
if (eventOrigin === merchantOrigin && eventName === 'PROPS_UPDATE' && newProps && typeof newProps === 'object') { | |
// send event ack so PostMessenger will stop reposting event | |
sendEventAck(id, merchantOrigin); | |
const clientOrigin = decodeURIComponent(initialProps.origin); | |
if (eventOrigin === clientOrigin && eventName === 'PROPS_UPDATE' && newProps && typeof newProps === 'object') { | |
// send event ack so PostMessenger will stop reposting event | |
sendEventAck(id, clientOrigin); |
src/components/modal/v2/lib/utils.js
Outdated
|
||
export function sendEventAck(eventId, trustedOrigin) { | ||
// skip this step if running in test env because jest's target windows don't support postMessage | ||
if (window.process?.env?.NODE_ENV === 'test') { |
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.
did we perhaps mean?
if (window.process?.env?.NODE_ENV === 'test') { | |
if (process.env.NODE_ENV === 'test') { |
export function createUUID() { | ||
// crypto.randomUUID() is only available in HTTPS secure environments and modern browsers | ||
if (typeof crypto !== 'undefined' && crypto && crypto.randomUUID instanceof Function) { | ||
return crypto.randomUUID(); | ||
} | ||
|
||
const validChars = '0123456789abcdefghijklmnopqrstuvwxyz'; | ||
const stringLength = 32; | ||
let randomId = ''; | ||
for (let index = 0; index < stringLength; index++) { | ||
const randomIndex = Math.floor(Math.random() * validChars.length); | ||
randomId += validChars.charAt(randomIndex); | ||
} | ||
return randomId; | ||
} |
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 there a belter
util we can leverage that meets our needs here?
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.
belter has uniqueID
that returns uid_${randomID}_${timeID}
. Considerations are that the existing postMessenger message ids have a format that is different from that (32 alphanumeric vs the above format in hex); however, there is no validation for nor true consumer of the id we will generate here. Would you like me to use this belter uniqueID
?
let targetWindow; | ||
const popupCheck = window.parent === window; | ||
if (popupCheck) { | ||
targetWindow = window.opener; | ||
} else { | ||
targetWindow = window.parent; | ||
} |
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.
let targetWindow; | |
const popupCheck = window.parent === window; | |
if (popupCheck) { | |
targetWindow = window.opener; | |
} else { | |
targetWindow = window.parent; | |
} | |
const targetWindow = window.parent === window ? window.opener : window.parent; |
function listenAndAssignProps(newProps, propListeners) { | ||
Array.from(propListeners.values()).forEach(listener => { | ||
listener({ ...window.xprops, ...newProps }); | ||
}); | ||
Object.assign(window.xprops, newProps); | ||
} |
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.
The name of this function makes it sound like we are creating a listener of some sort when in reality we calling the listeners that have already been registered. Consider avoiding the listen
verb as part of the function name.
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.
How about handleNewProps
or handleUpdatedProps
?
const [k, v] = entry; | ||
if (k === 'offerTypes') { | ||
validatedProps.offer = validate.offer({ props: { offer: v } }); | ||
} else { | ||
validatedProps[k] = validate[k]({ props: { [k]: v } }); | ||
} |
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.
Are we guaranteed to only pass in props that we have validation for? Should we account for the scenario where validate[k]
is undefined
which would throw an error here.
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.
Good call. See #1170 for this fix
…components into feature/DTCRCMERC-3611-modal-lander-v6
Description
Updates the modal lander to support communicating bi-directionally with the v6 modal wrapper
Screenshots
Testing instructions
feat: add learn more modal wrapper for messages
in an internal repo (DM Dan for more details).LearnMore.ts
, and on line 52, append&stage_tag=merc3611_test
to end of url query.