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
LPS-113532 Modernize Liferay.Util.getTop()
#2056
Conversation
CI is automatically triggering the following test suites:
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: getTop 1 Successful Jobs:For more details click here. |
Jenkins Build:test-portal-source-format#1087 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#2056 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - kresimir-coko > liferay-frontend - PR#2056 - 2022-03-25[02:30:38] Testray Importer:publish-testray-report#4031 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#1494 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#2056 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - kresimir-coko > liferay-frontend - PR#2056 - 2022-03-25[02:30:33] Testray Importer:publish-testray-report#4036 |
ci:test:relevant |
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.
This looks like the correct move of where this code should live, moving it out from aui-web and into js-web. However, I noticed the logic of the function changed a bit and I don't think it is the same as it was before.
I would just copy the original code, and update the references to Util._topWindow and reference a local variable of topWindow in the file.
let _topWindow;
function getTop() {
// ...
_topWindow = topWindow;
}
return topWindow;
}
return window; | ||
} | ||
|
||
while (parentWindow) { |
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 previous code had
while (parentWindow !== window) {
I think this change might actually create some bugs as it is not the same logical flow. I know you add the check above this block, but I think we actually need that in the while loop as the parentWindow is being set to a different value on each iteration.
*/ | ||
|
||
export default function getTop() { | ||
const SIMULATION_DEVICE_IFRAME = 'simulationDeviceIframe'; |
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.
This can be hoisted outside of the function as it is a constant
ci:stop |
❌ ci:test:relevant - 0 out of 1 jobs passed in 32 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 9b640c3cfb997f3512a83ae79c1e24b44224f27e ci:test:relevant - 0 out of 1 jobs PASSEDFor more details click here.Test bundle downloads:
|
Jenkins Build:test-portal-source-format#1114 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#2056 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - kresimir-coko > liferay-frontend - PR#2056 - 2022-03-30[05:31:06] Testray Importer:publish-testray-report#4132 |
Unexpected errors occurred while attempting to forward pull request to 'brianchandotcom', please try again later or contact a CI Infrastructure member for assistance. |
1 similar comment
Unexpected errors occurred while attempting to forward pull request to 'brianchandotcom', please try again later or contact a CI Infrastructure member for assistance. |
Error has occurred while forwarding pull request to |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: getTop 1 Successful Jobs:For more details click here. |
Jenkins Build:test-portal-source-format#1969 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#2056 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - kresimir-coko > liferay-frontend - PR#2056 - 2022-03-31[01:57:48] Testray Importer:publish-testray-report#7249 |
All required test suite(s) passed. |
Pull request has been successfully forwarded to brianchandotcom#115501 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#1376 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#2056 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - kresimir-coko > liferay-frontend - PR#2056 - 2022-03-31[02:04:47] Testray Importer:publish-testray-report#3321 |
LPS in question: https://issues.liferay.com/browse/LPS-113532
This one was a bit of a hassle due to testing woes around the Window object. The implementation itself is kinda whatever, nothing fancy.
This util usually gets invoked in modals/popups to retrieve the proper Window object needed for further work in the app.