Fix aliased default options #567
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you, @tbekolay! I spotted this error while I was refactoring some things and have already fixed it on the master branch. I've created a "v1.3-bugfix" branch so if you change the merge target from "master" to "v1.3-bugfix", then I can accept this pull request and release |
Because extend does a shallow copy of object properties, some options in interactable objects were aliased to objects in the default objects. In some cases, setting options on one interactable would bleed over to interactables made subsequently. This commit adds a clone utility which is similar to extend, but makes new objects instead of aliasing them. This allow the newly added tests to pass.
tbekolay
force-pushed
the
fix-peraction-alias
branch
from
3e88b82 to
7db5cc4
Compare
|
That all sounds great, thanks! I changed the target of this PR and rebased my branch to I had to make a small change to my test, but it passes now; unfortunately I get a test failure in |
taye
pushed a commit
that referenced
this pull request
Jan 2, 2018
Because extend does a shallow copy of object properties, some options in interactable objects were aliased to objects in the default objects. In some cases, setting options on one interactable would bleed over to interactables made subsequently. This commit adds a clone utility which is similar to extend, but makes new objects instead of aliasing them. This allow the newly added tests to pass. Re: #567
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I recently updated to interact.js 1.3.0 so I could use
restrictSizeandrestrictEdgeswhen resizing. However, when I updated, I got the following behavior when resizing an element that was working correctly with version 1.2.9.Resizing from the top edge is completely messed up. I tried to reproduce this in isolation in this codepen, but in isolation this worked fine.
I dug a little deeper, and as it turned out, the top edge was being snapped down to nothing because of what was set in
restrictEdges.outer. I found this odd because I had not setrestrictedEdges.outerfor this svg rect. However, I had setrestrictEdges.outerin another file. As it turns out, I had setrestrictEdges.outerto an element that was on the bottom of the screen, and since this setting bled out to the svg rect, resizing from the top immediately tried to respect the restriction, which was at the very bottom of the page.This was an annoying bug to track down, but fortunately it was relatively easy to figure out the fix. In
Interactable.js,extendis used in several places to copy defaults. However,extenddoes not make a copy of what it is extending, which in this specific case was causing my problem. The per action default object was being set (aliased) and then modified by the first interactable.While the bug was somewhat easy to fix, it was unfortunately not very easy for me to test. I could not find another test that made an
Interactable, so I did my best to make one. It took quite a few iterations to get something that would run and reproduce the error. I will defer to you, @taye, to fix up the test if it could be made better. But it would be great to have the bug fixed so I can userestrictEdgesandrestrictSizein my project :)