feat(annotations): marker body with dynamic positioning #1116

nickofthyme · 2021-04-12T21:36:37Z

Summary

Adds a new render component markerBody on LineAnnotationSpec which is positioned using popper.js within the chart <canvas> area. markerBody and marker (i.e. the marker icon) are now able to render as ReactNode or a ComponentType<LineAnnotationDatum>. This allows for simple text or JSX components or a complex react component that is rendered with the LineAnnotationDatum as props. With these props one can render a marker icon or body based on values from the datum.

Screen.Recording.2021-04-19.at.04.38.49.PM.mp4

Note: The markerBody is only rendered when a marker (aka icon) is provided.

Refactoring notes:

Abstracted the marker logic from annotations into LineMarker component.
Create a eachRotation helper to DRY-up code for it.each<Rotation> logic where -90 in a png filename is converted to 90, unknowingly causing duplicated png file names to be overridden.

Add index to annotation id in the likely event there are multiple annotations with the same joined parameters as seen in the code below...

elastic-charts/src/chart_types/xy_chart/annotations/line/dimensions.ts

Lines 407 to 415 in 4bf2ab1

    
           export function getAnnotationLinePropsId( 
        
             specId: string, 
        
             datum: LineAnnotationDatum, 
        
             index: number, 
        
             verticalValue?: any, 
        
             horizontalValue?: any, 
        
           ) { 
        
             return [specId, verticalValue, horizontalValue, datum.header, datum.details, index].join('__'); 
        
           }

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

nickofthyme · 2021-04-12T21:38:26Z

src/utils/common.tsx

@@ -541,7 +558,7 @@ export function isDefined<T>(value?: T): value is NonNullable<T> {
 *
 * @internal
 */
-export const isDefinedFrom = <T>(typeCheck: (value: RecursivePartial<T>) => boolean) => (
+export const isDefinedFrom = <T,>(typeCheck: (value: RecursivePartial<T>) => boolean) => (


Converting this from *.ts to *.tsx file, the generic const function declaration has issues.

This is a convenient fix that does not affect the type of the generic.

See https://stackoverflow.com/questions/32308370/what-is-the-syntax-for-typescript-arrow-functions-with-generics

What if instead of having this hack (as described in the stackoverflow comment) we transform these to named functions?

nickofthyme · 2021-04-19T21:46:25Z

src/chart_types/xy_chart/renderer/canvas/styles/area.test.ts

+import * as common from '../../../../../utils/common';
 import { buildAreaStyles } from './area';

 jest.mock('../../../../../common/color_library_wrappers');
-jest.mock('../../../../../utils/common');
+jest.spyOn(common, 'getColorFromVariant');


I'm not sure how or why my changes to utils/common.ts(x) affected this but this caused mocks to return undefined. This changes the logic to now only spys on the common#getColorFromVariant utils module method for conducting mock returns in styles unit tests.

markov00

LGTM. Tested locally and works fine.
I've few doubts about the "padding" comments on the story, but I think it's fine leaving it like that. Probably a different background for the annotation or pushing the annotation text and line after the axis labels can be useful

markov00 · 2021-04-20T11:57:26Z

src/utils/common.tsx

@@ -541,7 +558,7 @@ export function isDefined<T>(value?: T): value is NonNullable<T> {
 *
 * @internal
 */
-export const isDefinedFrom = <T>(typeCheck: (value: RecursivePartial<T>) => boolean) => (
+export const isDefinedFrom = <T,>(typeCheck: (value: RecursivePartial<T>) => boolean) => (


What if instead of having this hack (as described in the stackoverflow comment) we transform these to named functions?

src/chart_types/xy_chart/renderer/dom/annotations/annotations.tsx

src/chart_types/xy_chart/renderer/dom/annotations/line_marker.tsx

src/chart_types/xy_chart/utils/specs.ts

markov00 · 2021-04-20T12:16:56Z

src/chart_types/xy_chart/renderer/dom/annotations/line_marker.tsx

+  const setPopper = useCallback(() => {
+    if (!iconRef.current || !testRef.current) return;
+


On a different PR we should extract the popper.js to be a HOF or something similar to enhance the positioning of everything that we need

That's not a bad idea. I wonder if the logic is too bespoke for a HOC, I could see a class. Anyway, happy to discuss this in the future. I also secretly want to test popper on eui and see if it's better performance 🤪, we'll see if I ever have the time.

# [29.0.0](v28.2.0...v29.0.0) (2021-04-22) ### Features * **a11y:** add label for screen readers ([#1121](#1121)) ([920e585](920e585)), closes [#1096](#1096) * **annotations:** marker body with dynamic positioning ([#1116](#1116)) ([601abac](601abac)) ### BREAKING CHANGES * **a11y:** `description` prop in `<Settings/>` is renamed to `ariaDescription` Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

nickofthyme · 2021-04-22T19:56:58Z

🎉 This PR is included in version 29.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [29.0.0](elastic/elastic-charts@v28.2.0...v29.0.0) (2021-04-22) ### Features * **a11y:** add label for screen readers ([opensearch-project#1121](elastic/elastic-charts#1121)) ([ddb8782](elastic/elastic-charts@ddb8782)), closes [opensearch-project#1096](elastic/elastic-charts#1096) * **annotations:** marker body with dynamic positioning ([#1116](elastic/elastic-charts#1116)) ([997d487](elastic/elastic-charts@997d487)) ### BREAKING CHANGES * **a11y:** `description` prop in `<Settings/>` is renamed to `ariaDescription` Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

fix(annotations): marker wrapping

e25f1c5

nickofthyme added bug Something isn't working enhancement New feature or request :annotation Annotation (line, rect, text) related issue labels Apr 12, 2021

nickofthyme requested a review from markov00 April 12, 2021 21:36

nickofthyme commented Apr 12, 2021

View reviewed changes

nickofthyme added 9 commits April 12, 2021 16:43

chore: update api.md

544fd33

Merge branch 'master' into fix-anno-marker-wrap

9cec605

Merge branch 'master' into fix-anno-marker-wrap

59cf29f

fix: logic for left, right and any rotation

0accb86

fix: annotation right axis icon translation

81b52d6

chore: add note about explicit padding

7f0efe9

chore: cleanup repeated logic around rotations using .each in vrt

62660e5

chore: add test cases for different rotations and sides

ebac50f

test: fix broken tests and update types

c7806cc

nickofthyme marked this pull request as ready for review April 19, 2021 21:37

Merge branch 'master' into fix-anno-marker-wrap

4bf2ab1

nickofthyme commented Apr 19, 2021

View reviewed changes

markov00 approved these changes Apr 20, 2021

View reviewed changes

nickofthyme added 4 commits April 20, 2021 08:35

chore: prefer fn delcarations

18fe011

chore: update prop descriptions

070d031

fix: simplify annotation id

e8f559e

fix: remove css file and update vrt with default padding

cc33213

nickofthyme enabled auto-merge (squash) April 21, 2021 14:45

nickofthyme changed the title ~~feat(annotations): marker wrapping~~ feat(annotations): marker body with dynamic positioning Apr 21, 2021

nickofthyme added 4 commits April 21, 2021 09:56

fix: update vrt screenshots

4205f55

fix: vrt screenshots

788b681

ci: fix icon loading for vrt

8432949

Merge branch 'master' into fix-anno-marker-wrap

62f8841

nickofthyme merged commit 601abac into elastic:master Apr 22, 2021

nickofthyme added the released Issue released publicly label Apr 22, 2021

nickofthyme deleted the fix-anno-marker-wrap branch September 18, 2021 20:20

nickofthyme mentioned this pull request Sep 19, 2023

fix(tooltip): rendering in react v18 #2169

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(annotations): marker body with dynamic positioning #1116

feat(annotations): marker body with dynamic positioning #1116

nickofthyme commented Apr 12, 2021 •

edited

Loading

nickofthyme Apr 12, 2021

markov00 Apr 20, 2021

nickofthyme Apr 19, 2021

markov00 left a comment

markov00 Apr 20, 2021

markov00 Apr 20, 2021

nickofthyme Apr 20, 2021

nickofthyme commented Apr 22, 2021

	export function getAnnotationLinePropsId(
	specId: string,
	datum: LineAnnotationDatum,
	index: number,
	verticalValue?: any,
	horizontalValue?: any,
	) {
	return [specId, verticalValue, horizontalValue, datum.header, datum.details, index].join('__');
	}

		const setPopper = useCallback(() => {
		if (!iconRef.current \|\| !testRef.current) return;

feat(annotations): marker body with dynamic positioning #1116

feat(annotations): marker body with dynamic positioning #1116

Conversation

nickofthyme commented Apr 12, 2021 • edited Loading

Summary

Checklist

nickofthyme Apr 12, 2021

Choose a reason for hiding this comment

markov00 Apr 20, 2021

Choose a reason for hiding this comment

nickofthyme Apr 19, 2021

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Apr 20, 2021

Choose a reason for hiding this comment

markov00 Apr 20, 2021

Choose a reason for hiding this comment

nickofthyme Apr 20, 2021

Choose a reason for hiding this comment

nickofthyme commented Apr 22, 2021

nickofthyme commented Apr 12, 2021 •

edited

Loading