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

fix(demo): fix delaunay-voronoi demo #1758

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(demo): fix delaunay-voronoi demo #1758

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1462a22
fix(visx-demo): fix setHoveredId in delaunay-voronoi
kelvin27315 Oct 18, 2023
0d67dcd
fix(visx-demo): fix invalid bounds in delaunay-voronoi
kelvin27315 Oct 18, 2023
fb47cc7
Refactor to avoid early return and adhere to hooks rules
kelvin27315 Oct 20, 2023
07a06d7
Merge branch 'airbnb:master' into fix-voronoi-demo
kelvin27315 Nov 27, 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
Next Next commit
fix(visx-demo): fix setHoveredId in delaunay-voronoi
  • Loading branch information
@kelvin27315
kelvin27315 committed Oct 18, 2023
commit 1462a224463a7d16f8437768718c8b89d77c8a5a
2 changes: 1 addition & 1 deletion packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function Example({ width, height, margin = defaultMargin }: VoronoiProps) {

const closest = voronoiDiagram.delaunay.find(point.x, point.y);
// find neighboring polygons to hightlight
if (closest && data[closest].id !== hoveredId) {
if (data[closest].id !== hoveredId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is find guaranteed to return something? if closest could be undefined, this would then throw an error when evaluating data[closest].id

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the voronoiDiagram.delaunay.find(point.x, point.y); line, the find method of the Delaunay class from d3-delaunay is being used, which is defined as follows in @types/d3-delaunay, guaranteeing that a number type will always be returned according to the type definition:

/**
 * Returns the index of the input point that is closest to the specified point ⟨x, y⟩.
 * The search is started at the specified point i. If i is not specified, it defaults to zero.
 */
find(x: number, y: number, i?: number): number;

DefinitelyTyped Definition

Furthermore, the actual d3-delaunay implementation is as follows: if ((x = +x, x !== x) || (y = +y, y !== y)) return -1; checks if x and y are numbers, and returns -1 if they are not. However, since this Demo is adopting TypeScript, I believe there should be no issue as long as it adheres to TypeScript's type checking.

find(x, y, i = 0) {
  if ((x = +x, x !== x) || (y = +y, y !== y)) return -1;
  const i0 = i;
  let c;
  while ((c = this._step(i, x, y)) >= 0 && c !== i && c !== i0) i = c;
  return c;
}

d3-delaunay Implementation

Additionally, this delaunay.find is already used as follows in the demo of visx-delaunay-triangulation, and there isn’t a check for undefined there.

const closest = delaunayDiagram.find(point.x - margin.left, point.y - margin.top);
setHoveredId(data[closest].id);

visx-delaunay-triangulation Demo

const neighbors = Array.from(voronoiDiagram.neighbors(closest));
setNeighborIds(new Set(neighbors.map((d) => data[d].id)));
setHoveredId(data[closest].id);
Expand Down