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

refactor(bullet): better tooltip foundations #1246

Merged
merged 22 commits into from
Jul 15, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jul 10, 2021

Summary

Refactoring of the bullet/gauge/goal implementation such that it's straightforward to obtain bounding box information for solving #1187

Details

This PR moves the geom primitives generation out of the terminal processing step (renderer) and puts it into the data flow for reusability. This is achieved by forming an interface for primitive geoms, called Mark which might have various properties but must have a render method.

This required that the hierarchical scenegraph structure, done with nested withContexts and within them, modular transforms, get flattened, so it's not just a code shuffle. The small pixel differences are due to flattened mark positioning. Only the devicePixelRatio correction is retained, and even the center transform and the Y flip transform are gone.

The purpose of this refactoring is, besides Marco suggesting it a long time ago, the subsequent addition of a getBoundingBox method, which, min/maxed over all the geoms, yields the overall bounding box, which can let us easily fulfill #1187 (the pickQuad method can rely on the new selector and the bounding box reduction to decide if it's part of the bbox of the inked area, plus some padding). Future, sophisticated reuse is also possible, eg. different tooltip per hovered band (see #1218 (comment))

Issues

The baseline toward fulfilling #1187

Checklist

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
  • Whenever possible, please check if the closing issue is connected to a running GH project
  • Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility

@monfera monfera added enhancement New feature or request :goal/gauge (old) Old Goal/Gauge chart related issues labels Jul 10, 2021
@monfera monfera marked this pull request as draft July 10, 2021 09:43
@monfera monfera force-pushed the bullet-viewmodel branch from 89af187 to fd395d6 Compare July 14, 2021 09:46
@monfera monfera marked this pull request as ready for review July 14, 2021 17:47
@monfera monfera force-pushed the bullet-viewmodel branch from fd395d6 to d475d20 Compare July 14, 2021 17:49
@monfera monfera changed the title feat(bullet): better tooltip foundations refactor(bullet): better tooltip foundations Jul 14, 2021
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Changes looks good to me.
I was just wondering if we can use curried rendering functions instead of classes.
It looks way cleaner with classes, but I just wonder if we can have some benefits without them:

// instead of
return new Section(x0, y0, x1, y1, lineWidth, strokeStyle);
...
geomObjects.forEach((obj) => withContext(ctx, (ctx) => obj.render(ctx)));
// this
return sectionRender(x0, y0, x1, y1, lineWidth, strokeStyle);
...
geomObjects.forEach((obj) => withContext(ctx, (ctx) => obj(ctx)));

@monfera
Copy link
Contributor Author

monfera commented Jul 15, 2021

@markov00 the classes are not cast in stone, and we can change now or in the future. These were my motives:

  1. Besides the rendering, other methods would join, right now, returning the bounding box, but who knows, just a thought experiment, maybe we could have such marks or sets of marks (eg. a bunch of points) have methods for Canvas, SVG, HTML and WebGL rendering (your proposed closure approach could solve multiple methods too, eg. obj().render(ctx) or something, but at that point I'm duplicating class functionality)
  2. Using "proper" methods, there was no need for eg. type guard is functions, which would be a necessity with plain objects (I realize the closures you propose solve it too); in general, class instances are the only structured thing in JavaScript that let you enjoy both static typing (TS) and runtime typing (method dispatch, or instanceof check) without awkward type guard functions and associated runtime and code complexity costs

Also, while I'm not fond of classes in general, here it's super shallow, there's no inheritance, no mutation, no internal statefulness. It's possible to reconsider it and move away from it. Though I didn't think of classes as a contentious point, because components and Cartesian scales are already class clad

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM tested locally chrome

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Changes look fine to me.

@monfera monfera merged commit 8e66e13 into elastic:master Jul 15, 2021
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 33.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :goal/gauge (old) Old Goal/Gauge chart related issues released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants