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.

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

Merged
merged 4 commits into from
Mar 26, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Mar 18, 2018

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:

  • add onLoad utility to wait for window load even after it is already loaded
  • fix various component listeners on window load when the window is already loaded

Notes:

  • In addition to the bug fix, Accordion now smoothly scroll to the opened pane when the hash is changed
  • When a Magellan component is dynamically created, the window DO NOT scroll to the target corresponding to the location hash as the magellan did not created the target and should appear without effects on it after the page already loaded.

…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
*/
function onLoad(fn) {
if (document.readyState === 'complete')
setTimeout(() => fn(), 0);
Copy link
Contributor

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.

Copy link
Contributor Author

@ncoden ncoden Mar 19, 2018

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.

Copy link
Contributor

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 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@ncoden ncoden Mar 21, 2018

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.

@ncoden
Copy link
Contributor Author

ncoden commented Mar 23, 2018

@DanielRuf I fixed the bug in tests caused by the onLoad event that was not cleared when the component was destroyed (so the event was called after the component is destroyed, with an empty this). Now I use an event in both cases, load or the custom _didLoad returned by onLoad so we can use:

const listener = onLoad($elem, handler);
...
$elem.off(listener);

@ncoden
Copy link
Contributor Author

ncoden commented Mar 24, 2018

Thanks for the review.

@ncoden ncoden merged commit b860130 into foundation:develop Mar 26, 2018
@ncoden ncoden deleted the fix/on-load-listeners branch April 7, 2018 17:45
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…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>
ncoden added a commit that referenced this pull request Jun 27, 2018
fix: resolve invalid conflict resolution #11077 for v6.5
@ncoden ncoden mentioned this pull request Jul 8, 2018
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants