-
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: support dynamically added components #8016 #10947
Conversation
Working codepen: https://codepen.io/DanielRuf/pen/LQOWmE |
I just found the same issue with |
Right, we may have to add this to some more components. |
I'll check the other components, prepare an issue and collect them there and link the PRs then (I would rather go step for step or component for component to have better and more atomic changes which is easier for bisecting and regression testing imo). |
I was going to take care of this and open a PR but ok, have fun ;) |
Agreed. I'll prepare the other commits then. If you want you can also push to my branch. Or do we already have some PRs for the other components? |
I don't think. There was PRs before that resolved some cases manually (and sometimes badly), but everything is merged by now. |
Also see #9047 |
Thanks. |
Pushed a new commit. Did not find more occurences where we wait for the |
@DanielRuf Seen. LGTM but require some cleaning as said at #10988 (comment), maybe in an other PR (that or in that PR but with a squash after). Do you want to take care of this or should I ? |
Would be glad if I could get some assistance here so feel free to prepare a PR with the needed changes to make them all consistent then. |
_this.calcPoints(); | ||
_this._updateActive(); | ||
}); | ||
} |
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.
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.
@DanielRuf What do you think ?
Closing in favor of #11077 |
We have to check for
document.readyState === "complete"
when the window and content is already loaded and more content is loaded afterwards that is needed for the magellan and sticky solution.