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.

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

feat: add ability to hover, select, and filter points upon draw() #142

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

flekschas
Copy link
Owner

@flekschas flekschas commented Jun 23, 2023

This PR adds the ability to hover, select, and filter points at the time when draw() is called.

Description

What was changed in this pull request?

I added three new properties to the draw options: hover, select, and filter. Those options allow to immediately hover, select, or filter points as they are drawn. E.g.:

const points = [[0, 0], [0.5, 0.5], [1, 1]];
scatterplit.draw(points, { hover: 0, select: [0, 2], filter: [0, 1] });

In the above example, upon drawing the three points, the first point is hovered and selected and the last point is filtered out. While we also attempted to select the last point it's not going to be selected because we filter it out.

For convenience, this PR also expose hoveredPoint via scatterplot.get('hoveredPoint').

Why is it necessary?

Previously, one had to filter after the draw call which leds to flickering as shown in the gif.

const points = generatePoints(numPoints);

const filter = points.x.map((x, i) => [x, i]).filter(([x]) => x < 0).map(([x, i]) => i);

const filterSet = new Set(Array.from({ length: filter.length }, (_, i) => i));
const select = [];
for (let i = 0; i < 20; i++) {
  const idx = Math.floor(Math.random() * filterSet.size);
  select.push(filter[idx]);
  filterSet.delete(idx);
}

const hover = select[Math.floor(Math.random() * select.length)];

scatterplot.draw(points).then(() => {
  // Since we first have to wait until the points are drawn before we can hover, select, and filter
  // there's a split second where all points are shown
  scatterplot.filter(filter);
  scatterplot.select(select);
  scatterplot.hover(hover);
});

filter-hover-select-after-draw

Now, one can directly hover, select, and filter at the time of drawing the points, which avoid the flickering.

const points = generatePoints(numPoints);

const filter = points.x.map((x, i) => [x, i]).filter(([x]) => x < 0).map(([x, i]) => i);

const filterSet = new Set(Array.from({ length: filter.length }, (_, i) => i));
const select = [];
for (let i = 0; i < 20; i++) {
  const idx = Math.floor(Math.random() * filterSet.size);
  select.push(filter[idx]);
  filterSet.delete(idx);
}

const hover = select[Math.floor(Math.random() * select.length)];

scatterplot.draw(points, { hover, select, filter });

draw-with-filter-hover-select

Checklist

  • Provided a concise title as a semantic commit message (e.g. "fix: correctly handle undefined properties")
  • CHANGELOG.md updated
  • Tests added or updated
  • Documentation in README.md added or updated
  • Example(s) added or updated
  • Screenshot, gif, or video attached for visual changes

Also expose `hoveredPoint` via `scatterplot.get('hoveredPoint')`
@flekschas flekschas added the improvement Feature improvement or enhancement label Jun 23, 2023
@flekschas
Copy link
Owner Author

@insertmike Since we've talked about avoiding the flicker upon drawing new points and filtering them down, what do you think of this addition?

Copy link
Contributor

@insertmike insertmike left a comment

Choose a reason for hiding this comment

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

Good idea! @flekschas As the filter Issue occurred, I can imagine how it would as well for the added options, such as hover and select.

For convenience, this PR also expose hoveredPoint via scatterplot.get('hoveredPoint').

I think we should expose this in the README doc too. Otherwise we risk letting it die in the README. Except if this is built automatically by the build script (haven't looked at).

Lastly, I don't want to be pushy, we should take our time, but Is it possible to give an estimate on when we will make a release?

Comment on lines +163 to +165
hover: number;
select: number | number[];
focus: number | number[];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; The addition of the properties could benefit from more descriptive names such as hoveredPoints, selectedPoints, and focusedPoints to enhance clarity. I will let you decide here, as you know the codebase consistency better

Copy link
Owner Author

Choose a reason for hiding this comment

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

I will keep these prop names as is as they match the function names and internally the draw() function literally calls hover(), select(), or focus() instead of directly setting the state.

Comment on lines +164 to +165
select: number | number[];
focus: number | number[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these 2 should always expect an array, it introduces additional complexity and potential vulnerabilities in the codebase. If it's selectedPoints, then it's clear for the users that they need to submit: [0] for example.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Both methods already support this signature. The complexity overhead is neglectable

@flekschas
Copy link
Owner Author

think we should expose this in the README doc too. Otherwise we risk letting it die in the README. Except if this is built automatically by the build script (haven't looked at).

Good call! I updated the README to reflect the new draw() signature

@flekschas flekschas merged commit 5106768 into master Jun 27, 2023
@flekschas flekschas deleted the enhance-draw-with-hover-select-filter branch June 27, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Feature improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants