Skip to content
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.

Sign up for GitHub

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

SAK-50797 LTI fix popup WC tag timing issue #13211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+9 −7
Open

SAK-50797 LTI fix popup WC tag timing issue #13211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions webcomponents/tool/src/main/frontend/packages/sakai-lti-popup/src/SakaiLTIPopup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export class SakaiLTIPopup extends SakaiElement {
this.preLaunchText = null;
this.postLaunchText = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The way you are testing for these being null in the i18n promise sets up a race condition. The setting of attributes happens after the constructor, but that setting may happen before the i18n promise fulfils. So, a user of the tag may think they are overriding the i18n default, but they maybe won't.

this.auto = false;
this.popped = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to set this to false. It is initially undefined which is already falsy

this.loadTranslations("lti").then(t => {

this._i18n = t;
Expand All @@ -29,21 +30,22 @@ export class SakaiLTIPopup extends SakaiElement {
}

launchPopup() {

window.open(this.launchUrl, "_blank");
document.getElementById("sakai-lti-popup-" + this.randomId).style.display = "none";
document.getElementById("sakai-lti-popup-hidden-" + this.randomId).style.display = "block";
return false;
}

shouldUpdate() {
return this.preLaunchText && this.postLaunchText && this.launchUrl;
shouldUpdate(changedProperties) {
return changedProperties.has("preLaunchText") || changedProperties.has("postLaunchText") ||
changedProperties.has("launchUrl") || changedProperties.has("auto");
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not need to do any of this because these properties are already reactive. All you are doing here is repeating lit's internal logic when any one of these four reactive properties is changed.

}

firstUpdated(changedProperties) {
changedProperties.forEach((oldValue, propName) => {
if ( propName == "auto" && this.auto ) setTimeout(this.launchPopup(), 1000);
});
firstUpdated() {
if ( this.auto && ! this.popped ) {
this.popped = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that if you make this reactive and just use this.popped in your template, you wont need this timeout at all. Also, you don't need the randomid if you use this.querySelector because it's scoped for you already.

setTimeout(this.launchPopup(), 1000);
}
}

render() {
Expand Down
Loading