refactor(bullet): better tooltip foundations#1246
Conversation
89af187 to
fd395d6
Compare
fd395d6 to
d475d20
Compare
markov00
left a comment
There was a problem hiding this comment.
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)));|
@markov00 the classes are not cast in stone, and we can change now or in the future. These were my motives:
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 |
rshen91
left a comment
There was a problem hiding this comment.
LGTM tested locally chrome
nickofthyme
left a comment
There was a problem hiding this comment.
Changes look fine to me.
|
🎉 This PR is included in version 33.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
Markwhich might have various properties but must have arendermethod.This required that the hierarchical scenegraph structure, done with nested
withContexts and within them, modulartransforms, get flattened, so it's not just a code shuffle. The small pixel differences are due to flattened mark positioning. Only thedevicePixelRatiocorrection 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
getBoundingBoxmethod, which, min/maxed over all the geoms, yields the overall bounding box, which can let us easily fulfill #1187 (thepickQuadmethod 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
packages/charts/src/index.ts(and stories only import from../srcexcept for test data & storybook)