-
-
Notifications
You must be signed in to change notification settings - Fork 646
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 retethering issue when anchor refs are removed #95
Conversation
if (typeof selector === 'string') { | ||
opts.element = document.querySelector(selector); | ||
returnOpts.on = opts.on; | ||
if (opts.offset !== 'undefined') { |
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.
opts.offset
really ends up the string 'undefined'
? That doesn't seem right (not saying your code is wrong, saying if that's the case, it should probably be fixed).
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.
oh good eye, i meant typeof
here
returnOpts.on = opts.on; | ||
if (typeof opts.offset !== 'undefined') { | ||
returnOpts.offset = opts.offset; | ||
} |
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.
I think you can just do returnOpts = extend({}, opts)
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.
You're right - I got this confused with something else, it's only a single level of props so i wouldn't need deep copy anyway.
@zackbloom anything else need tweaking before this is merged? |
👍 |
@zackbloom im going to merge this |
Fix retethering issue when anchor refs are removed
Publishing as |
@zackbloom
I am working on a hubspot app that has a very dynamic, multi-page onboarding flow. The problem happens after the user finishes a flow and then decides to start it over. This situation might apply more broadly as well.
Current behavior (attaches to 0,0)
Fixed behavior (attaches to element)
The issue comes from the fact that the user options
opts.element
reference is overwritten when setting the element reference usingdocument.getQuerySelector
and stays in memory. Unless the steps are explicitly removed and added again (which, in practice, is not intuitive) the elements are never relieved of their reference and thus will always re-attach to the historical first reference that has no presence on the page.(Additionally, a deepNevermind that doesn't apply hereextend
would have worked nicely here but it seems like that doesn't happen in Tether utils).