-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix: fix "load" actions in dynamically added components #11077
Conversation
…ready loaded Prevent bugs in Accordion, Magellan, OffCanvas, Reveal, Sticky, Tabs and trigger utilities when the component is dynamically added because the window `load` event they rely on would never be triggered. Note: In addition to the bug fix, the following behaviors are changed: * Accordion now smoothly scroll to the opened pane when the hash is changed
js/foundation.util.core.js
Outdated
*/ | ||
function onLoad(fn) { | ||
if (document.readyState === 'complete') | ||
setTimeout(() => fn(), 0); |
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 should be fn();
as setTimeout
creates a new context and does not bind _this
and nother variables in this case.
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.
There is no context to preserve. _this
and other variables are not passed through the context.
We need setTimeout
here to keep when same behavior (asynchronous call) when the document is ready or not.
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.
Take a look at the failing unit test(s) due to this ;-)
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.
Right, but this has nothing to do with the context, as this
is not used.
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 error comes from the component being already destroyed when the function in setTimeout
is executed. I don't exactly know why, but the best solution would be to handle this case as we do for others events listeners: to unregister it on destroy
.
@DanielRuf I fixed the bug in tests caused by the const listener = onLoad($elem, handler);
...
$elem.off(listener); |
Thanks for the review. |
…or v6.5.0 f59e95c feat: add utility to wait for page load even after it is already loaded f353fc2 fix: fix various component listeners on page load when the page is already loaded 13d9ca2 fix: refactor the uility to allow to unbind the created listenner Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
fix: resolve invalid conflict resolution #11077 for v6.5
Cleaner versions of #10947 - fix: support dynamically added components (@DanielRuf)
Prevent bugs in Accordion, Magellan, OffCanvas, Reveal, Sticky, Tabs and trigger utilities when the component is dynamically added because the window
load
event they rely on would never be triggered.Changes:
onLoad
utility to wait for window load even after it is already loadedNotes: