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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Jan 13, 2023
Merged

Add zoom functionality to wafermap #923

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
151764f
add initial implementation
ArinLucaNI Dec 14, 2022
33b7fdf
make zoomHandler non-static
arinluca333 Dec 14, 2022
139022c
Fixing transform issues.
atmgrifter00 Dec 14, 2022
8ff919b
fix zoom and add tests
arinluca333 Dec 15, 2022
41d6eae
name changes and removing unnecessary async tests
arinluca333 Dec 15, 2022
a58030e
add change file
arinluca333 Dec 15, 2022
4dfca2b
implement feedback
arinluca333 Dec 16, 2022
d4015ba
Update packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
arinluca333 Dec 16, 2022
232b1ee
change spelling
arinluca333 Dec 16, 2022
0eedc58
Merge branch 'users/aluca/add-zoom-functionality-to-wafermap' of http…
arinluca333 Dec 16, 2022
e1fb9a0
fix lint and implement feedback
arinluca333 Dec 19, 2022
abba7ff
pull latest
arinluca333 Jan 11, 2023
649c3a2
fix merge functionality
arinluca333 Jan 11, 2023
06391a8
implement feedback
arinluca333 Jan 11, 2023
bdfd705
fix lint
arinluca333 Jan 11, 2023
6e5c220
Update packages/nimble-components/src/wafer-map/index.ts
arinluca333 Jan 12, 2023
b4bd650
implement feedback
arinluca333 Jan 12, 2023
55d84f9
fix lint
arinluca333 Jan 12, 2023
489ce1c
fix tests and some minor feedback
arinluca333 Jan 13, 2023
09f4eae
pull latest and fix conflict
arinluca333 Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@ni-nimble-components-5067d35f-244c-42ff-b498-4329212f03ce.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
arinluca333 marked this conversation as resolved.
Show resolved Hide resolved
arinluca333 marked this conversation as resolved.
Show resolved Hide resolved
"type": "minor",
arinluca333 marked this conversation as resolved.
Show resolved Hide resolved
"comment": "Add zoom functionality to wafer map",
"packageName": "@ni/nimble-components",
"email": "49208904+arinluca333@users.noreply.github.com",
"dependentChangeType": "patch"
}
138 changes: 138 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/nimble-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
"@tanstack/table-core": "^8.7.0",
"@types/d3": "^7.4.0",
"@types/d3-scale": "^4.0.2",
"@types/d3-zoom": "^3.0.0",
"@types/d3-selection": "^3.0.0",
"d3-selection": "^3.0.0",
"d3-zoom": "^3.0.0",
"d3-random": "^3.0.1",
"d3-scale": "^4.0.2",
"hex-rgb": "^5.0.0",
Expand Down
27 changes: 23 additions & 4 deletions packages/nimble-components/src/wafer-map/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from './types';
import { DataManager } from './modules/data-manager';
import { RenderingModule } from './modules/rendering';
import { ZoomHandler } from './modules/zoom-handler';

declare global {
interface HTMLElementTagNameMap {
Expand Down Expand Up @@ -62,7 +63,12 @@ export class WaferMap extends FoundationElement {
/**
* @internal
*/
@observable public canvasSideLength: number | undefined;
public readonly zoomContainer!: HTMLElement;

/**
* @internal
*/
arinluca333 marked this conversation as resolved.
Show resolved Hide resolved
@observable public canvasSideLength?: number;

@observable public colorScaleMode: WaferMapColorScaleMode = WaferMapColorScaleMode.linear;

Expand All @@ -74,9 +80,10 @@ export class WaferMap extends FoundationElement {
};

private renderQueued = false;
private dataManager: DataManager | undefined;
private renderer: RenderingModule | undefined;
private resizeObserver: ResizeObserver | undefined;
private dataManager?: DataManager;
private renderer?: RenderingModule;
private resizeObserver?: ResizeObserver;
private zoomHandler?: ZoomHandler;
public override connectedCallback(): void {
super.connectedCallback();
this.resizeObserver = new ResizeObserver(entries => {
Expand All @@ -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(), {
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

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

passive: false
});
this.queueRender();
}

Expand Down Expand Up @@ -116,6 +126,14 @@ export class WaferMap extends FoundationElement {
this.maxCharacters
);
this.renderer = new RenderingModule(this.dataManager, this.canvas);
this.zoomHandler = new ZoomHandler(
this.canvas,
this.zoomContainer,
this.dataManager,
this.renderer,
this.canvasSideLength
);
this.zoomHandler.attachZoomBehavior();
this.renderer.drawWafer();
}

Expand Down Expand Up @@ -163,6 +181,7 @@ export class WaferMap extends FoundationElement {
this.canvas.width = this.canvasSideLength;
this.canvas.height = this.canvasSideLength;
}
this.zoomHandler?.resetTransform();
this.queueRender();
}

Expand Down
Loading