Skip to content

feat: supports v6 custom close button#1190

Merged
Seavenly merged 7 commits intofeature/modal-wrapper-eventsfrom
feature/modal-wrapper-close-btn
Apr 7, 2025
Merged

feat: supports v6 custom close button#1190
Seavenly merged 7 commits intofeature/modal-wrapper-eventsfrom
feature/modal-wrapper-close-btn

Conversation

@danzhaaspaypal
Copy link
Contributor

@danzhaaspaypal danzhaaspaypal commented Apr 1, 2025

Description

Logs modal close when triggered by the custom close button.
Also removes outdated focus code that interfered with giving custom close button focus.

Screenshots

Testing instructions

@danzhaaspaypal danzhaaspaypal requested a review from Seavenly April 3, 2025 19:06
@danzhaaspaypal danzhaaspaypal marked this pull request as ready for review April 4, 2025 14:42
Comment on lines +46 to +49
if (eventName === 'PROPS_UPDATE') {
handlePropsUpdateEvent(propListeners, event);
}
if (eventName === WRAPPER_CLOSE_MESSAGE_NAME) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably handle both of these event names the same to keep consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say handle them, do you mean how they are referenced? Either both as constants declared above or both as strings? If so do you have a preference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea exactly. I should have been a little more clear there. Since they are only referenced here I think just having the string values prevents an extra mental hop to the actual value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counterpoint, I kind of reference them across repos, by adding this at the top of the file here in the messaging repo...

// these constants should maintain parity with MESSAGE_MODAL_EVENT_NAMES in core-web-sdk
export const WRAPPER_CLOSE_MESSAGE_NAME = 'MODAL_CLOSED';

... and this at the top of LearnMore.ts in the core-web-sdk repo:

// these constants should maintain parity with POSTMESSENGER_EVENT_NAMES in paypal-messaging-components
// https://github.com/paypal/paypal-messaging-components/blob/3283443d3a970471aaf914761a0371a894c8d90a/src/components/modal/v2/lib/zoid-polyfill.js#L10
const WRAPPER_CLOSE_MESSAGE_NAME = 'MODAL_CLOSED'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I guess overall changing the constants to strings simplifies so let's go with that

@Seavenly Seavenly merged commit 8ad8237 into feature/modal-wrapper-events Apr 7, 2025
51 checks passed
@Seavenly Seavenly deleted the feature/modal-wrapper-close-btn branch April 7, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants