-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add zoom functionality to wafermap #923
Add zoom functionality to wafermap #923
Conversation
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
change spelling Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>
β¦s://github.com/ni/nimble into users/aluca/add-zoom-functionality-to-wafermap
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
@arinluca333 friendly reminder to click the "Re-request review" button for Milan and I when this is ready for us to take another look. That's the signal we'll use to know that you've addressed everything you plan to or you need more input from us. |
Thanks for the heads up! |
Co-authored-by: Milan Raj <rajsite@users.noreply.github.com>
@@ -86,6 +93,9 @@ export class WaferMap extends FoundationElement { | |||
this.canvasSideLength = Math.min(height, width); | |||
}); | |||
this.resizeObserver.observe(this); | |||
this.canvas.addEventListener('wheel', event => event.preventDefault(), { |
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.
We should remove this event listener in the disconnectedCallback
, otherwise we'll end up with duplicate listeners if the same element is added, removed, and re-added to the page.
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.
@arinluca333 I don't think the approach you used in removeEventListener()
will work. I believe the add/remove functions track the listener function by reference. You're attempting to remove a new listener function which is a copy of the original one. So removeEventListener won't find any listener that matches it and will silently fail.
From the MDN docs:
Calling removeEventListener() with arguments that do not identify any currently registered event listener on the EventTarget has no effect.
Instead you need to save the event listener to a class member variable and pass that variable to both addEventListener
and removeEventListener
.
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 filed #964 to you to track fixing 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.
Yep, the exact same function reference must be used, concretely (pseudocode):
class {
private wheelHandler = () => {};
connectedCallback() { this.canvas.addEventListener('wheel', wheelHandler);}
disconnectedCallback() { this.canvas.removeEventListener('wheel', wheelHandler); }
}
Pull Request
π€¨ Rationale
The wafer map needs to have a zoom functionality. This is provided by the d3 library.
π©βπ» Implementation
Added a d3 zoom event on the canvas that modifies the transform of the zoom-container.
π§ͺ Testing
Tested the change in the storybook and added test for where we simulate a zooming events.
β Checklist