Skip to content

Conversation

@malvpatel
Copy link
Contributor

@rouilj I have refactored according to the code review comments

const handlePopupReadyEvent = (event) => {
// we get a document Fragment in event.detail we replace it with the root
// replaceChild method consumes the documentFragment content, subsequent calls will be no-op.
if (e.detail.childElementCount === 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rouilj I have extensively checked this behavior in all browsers and subsequent calls will not have the documentFragment with content inside as it's consumed here with replaceChild.

Thus eliminating our need to debounce and worry about document fragments need to be destroyed.

replaceChild consumes the fragment content leaving it empty, and the reference to it also gets cleaned up when either we remove the listener or popup window is closed

}

ClassHelper.translations = translations;
ClassHelper.translations = json;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this is an artifact from another local branch that I was trying to solve, translations stopped working

This solves the problem where I was not attaching the API response to the member field

Copy link
Contributor

@rouilj rouilj left a comment

Choose a reason for hiding this comment

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

LGTM.

Please put this on main. I have to head out in a few minutes and will be at work tomorrow.
I'll do testing over the weekend.

@malvpatel malvpatel merged commit d1ba90d into main May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants