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: annotation marker size calculation #960

Merged
merged 13 commits into from
Jan 26, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jan 4, 2021

Summary

A propedeutic refactor of the LineAnnotation spec to allow the use of a custom react reconciler.

When using a custom reconciler, every component within the wrapping parent is treated in the same way. Due to the nature of the new reconciler we can't "host" mixed components that are both a Spec component (that store it's props into the redux store) and a React component (as it was the LineAnnotation where the marker was first drawn offscreen, measured and then stored into the redux store).

The LineAnnotation was refactored to ignore the marker size, the marker is just rendered when and where needed with a transformation to align itself based on its anchor point.

Note

  • The Path polyfill is removed from the dependencies, since we dropped the IE11 support there is no need for that anymore.
    The Path2D mock in the Enzyme testing was applied after receiving an error message from the testing environment. This should not affect the current unit testing as it's just used on the rendering side not tested through Enzyme.

  • The calculation of the marker size was used to compute the right position of the marker and the mouse coordinate hit point within that marker: I've replaced the placement with a simple css transform and I've replaced the computation of the mouse hit within the marker with a mouseenter/mouseleave listener on the marker container, storing the information of the hovered DOM element within the redux store.

  • I moved the computation of the formatted details/header within the tooltip since it was a very cleaner to just pass the original datum around and it derivated formatted form.

@codecov-io
Copy link

codecov-io commented Jan 4, 2021

Codecov Report

Merging #960 (31d943a) into master (fcc4994) will increase coverage by 1.64%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   70.90%   72.55%   +1.64%     
==========================================
  Files         350      350              
  Lines       11072    11052      -20     
  Branches     2437     2434       -3     
==========================================
+ Hits         7851     8019     +168     
+ Misses       3208     3019     -189     
- Partials       13       14       +1     
Flag Coverage Δ
unittests 72.55% <93.10%> (+1.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...chart/renderer/dom/annotations/tooltip_content.tsx 56.66% <50.00%> (+45.12%) ⬆️
.../xy_chart/renderer/dom/annotations/annotations.tsx 91.30% <90.47%> (+22.88%) ⬆️
...rt/state/selectors/get_annotation_tooltip_state.ts 88.88% <96.55%> (+6.74%) ⬆️
...hart_types/xy_chart/annotations/line/dimensions.ts 86.53% <100.00%> (-0.26%) ⬇️
...hart_types/xy_chart/annotations/rect/dimensions.ts 89.69% <100.00%> (-0.11%) ⬇️
...c/chart_types/xy_chart/annotations/rect/tooltip.ts 78.78% <100.00%> (-0.63%) ⬇️
src/chart_types/xy_chart/annotations/tooltip.ts 95.83% <100.00%> (-1.05%) ⬇️
...rt/renderer/dom/annotations/annotation_tooltip.tsx 89.65% <100.00%> (+27.58%) ⬆️
src/chart_types/xy_chart/specs/line_annotation.tsx 100.00% <100.00%> (+20.51%) ⬆️
src/state/actions/dom_element.ts 100.00% <100.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcc4994...e832bf4. Read the comment docs.

@markov00 markov00 added :specs Chart specifications related issue enhancement New feature or request labels Jan 19, 2021
@markov00 markov00 marked this pull request as ready for review January 19, 2021 17:23
@markov00 markov00 requested a review from nickofthyme January 19, 2021 17:24
if (hasMarkerDimensions) {
return undefined;
}
switch (alignment) {
Copy link
Contributor

@monfera monfera Jan 21, 2021

Choose a reason for hiding this comment

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

reason for changes: compact; O(1); no need for backquotes; no need to play tricks with a TS-enforced default: as Position can't be any other value. The constant can be moved outside the function, might even become reusable

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed here: 7465bb3

package.json Show resolved Hide resolved
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.

LGTM, left a few comments. Verified storybook line and rectangular annotation stories a still render and interact the same. 👍🏼

@monfera monfera self-requested a review January 25, 2021 15:48
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Great and quite compact PR, thanks Marco!

@markov00 markov00 merged commit c924310 into elastic:master Jan 26, 2021
@markov00 markov00 deleted the 2020_12_27-feat_react_reconciler_pt1 branch January 26, 2021 11:42
@markov00
Copy link
Member Author

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released Issue released publicly :specs Chart specifications related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants