Annotation label <foreignobject> render #1383

valtism · 2021-11-26T00:52:29Z

🚀 Enhancements

Add option to pass in an element to render as a label as a child.
Uses to allow user to pass in an HTML element.

Related to this discussion: #1173

This has the benefit of letting the element handle text reflow, with
the drawback being that the user need to manage min and max width of
the container element they render.

This approach seems much better imo, favouring composability over
configuration which is much more in line with the philosophy of d3.

Add option to pass in an element to render as a label as a child. Uses <foreignObject> to allow user to pass in an HTML element. Related to this discussion: airbnb#1173 This has the benefit of letting the element handle text reflow, with the drawback being that the user need to manage min and max width of the container element they render. This approach seems much better imo, favouring composability over configuration which is much more in line with the philosophy of d3.

Add witdth and height to <foreignObject>. Worked for Chrome but was not displaying anything for Safari.

Not sure how idomatic all the `dive()`s are, but I've heard complaints about them so I'm guess it's the way to go.

valtism · 2021-11-26T04:34:03Z

Example of the annotation in action. Notice the <div> handles text reflow much better. The label is also much easier to style how you want.

Screen.Recording.2021-11-26.at.3.31.39.pm.mov

@williaster Let me know what you think. Made this non-breaking as possible.

gazcn007

LGTM

williaster · 2022-01-04T21:53:47Z

hey @valtism , sorry I was away for all of Dec and am just getting back into the swing of things. will try to review/land this asap! (next day or two)

williaster

Woweee @valtism this one looks great 💯 thanks for adding this, it's a huge improvement for additional flexibility! 🔥 Can't wait to use it.

I had a couple suggestions, mostly about moving out the html-based label into its own file to support easier import and documentation.

williaster · 2022-01-19T23:09:07Z

packages/visx-annotation/src/components/Label.tsx

@@ -15,6 +15,8 @@ export type LabelProps = {
  backgroundPadding?: number | { top?: number; right?: number; bottom?: number; left?: number };
  /** Additional props to be passed to background SVGRectElement. */
  backgroundProps?: React.SVGProps<SVGRectElement>;
+  /** Pass in a custom element as the label to style as you like. Renders inside a <foreignObject> */


nit – period for docs.

Suggested change

/** Pass in a custom element as the label to style as you like. Renders inside a <foreignObject> */

/** Pass in a custom element as the label to style as you like. Renders inside a <foreignObject>. */

I wonder also if it'd be worth noting from the original issue something like

Note that copy/pasting content from a browser may not work if you use this option because not all non-browser SVG renderers render HTML <foreignObject>s

I'm actually not sure if this is true since the DOM tree in the implementation has a div <foreignObject><div>... so maybe don't have to worry about it for now. cc @richsilv

packages/visx-annotation/src/components/Label.tsx

Rename ForeignObjectLabel -> HtmlLabel Split out HtmlLabel and LabelAnchorLine to separate components. Export HtmlLabel as a separate component as it uses a small subset of the shared props.

valtism · 2022-01-20T02:09:38Z

@williaster thanks for the great feedback! I've incorporated everything you mentioned, but I'm not entirely sure how to test to see if the new HtmlLabel displays properly in the docs. Let me know if I missed anything.

williaster

thanks for iterating 🙌 docs look good!

fyi you can check by

rebuilding the package of interest yarn build:workspaces --workspaces=@visx/drag --esm --watch (the demo site pulls from the esm/ build, this is documented in CONTRIBUTING.md but is subtle)
start the demo site dev server yarn dev:demo (or from packages/visx-demo run yarn dev)
visit http://localhost:3000/docs/annotation (or whatever)

I had two minor comments based on playing with it a bit. I might follow up with adding an svg <> html toggle in the demo for easier testing, so I'm happy to make those two tweaks in that PR if you'd rather merge as is.

williaster · 2022-01-20T19:40:23Z

packages/visx-annotation/src/components/HtmlLabel.tsx

+      pointerEvents="none"
+      className={cx('visx-annotationlabel', className)}
+    >
+      {showAnchorLine && (


one thing I noticed while playing with this is that the line renders behind the HTML, do you think we should flip this and render the anchor line after foreignObject?

williaster · 2022-01-20T19:43:21Z

packages/visx-annotation/src/components/HtmlLabel.tsx

+        />
+      )}
+      <foreignObject width={width} height={height} overflow="visible">
+        <div ref={labelRef} style={wrapperStyle}>


when I was playing with this I found myself adding a bunch of styles to a div in children, I wonder if we should just expose this so users don't need an extra DOM element? something like containerStyle?: React.CSSProperties and then we can spread it in like style={containerStyle ? { ...wrapperStyle, containerStyle } : wrapperStyle

williaster · 2022-01-20T19:43:50Z

packages/visx-annotation/test/Label.test.tsx

-      shallow(
-        <Label title="title test" showBackground resizeObserverPolyfill={ResizeObserver} />,
-      ).find('rect'),
+      shallow(<Label title="title test" showBackground resizeObserverPolyfill={ResizeObserver} />)


🙏 thanks for fixing these

williaster · 2022-01-21T20:48:10Z

Hey @valtism I have some time today so am gonna land this and make those changes in another PR. Awesome job here! 🎉

github-actions · 2022-01-21T20:57:14Z

🎉 This PR is included in version v2.7.0 of the packages modified 🎉

valtism · 2022-01-23T23:45:25Z

@williaster Perfect. I'm not precious about my code, so thanks for making those fixes. Look forward to using it 😊

valtism added 4 commits November 26, 2021 11:51

Fix child render for Safari

ce5cd18

Add witdth and height to <foreignObject>. Worked for Chrome but was not displaying anything for Safari.

Fix test cases

a3bfc41

Not sure how idomatic all the `dive()`s are, but I've heard complaints about them so I'm guess it's the way to go.

Remove console log

9b9c60d

gazcn007 approved these changes Dec 18, 2021

View reviewed changes

williaster added the 🚀enhancement label Jan 4, 2022

williaster reviewed Jan 19, 2022

View reviewed changes

williaster added the @visx/annotation label Jan 19, 2022

Address PR comments

e1b5ef0

Rename ForeignObjectLabel -> HtmlLabel Split out HtmlLabel and LabelAnchorLine to separate components. Export HtmlLabel as a separate component as it uses a small subset of the shared props.

williaster approved these changes Jan 20, 2022

View reviewed changes

williaster added the minor-release label Jan 21, 2022

williaster merged commit ae47109 into airbnb:master Jan 21, 2022

williaster mentioned this pull request Jan 21, 2022

new(annotation): add HtmlLabel demo, add containterStyle prop #1424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation label <foreignobject> render #1383

Annotation label <foreignobject> render #1383

valtism commented Nov 26, 2021

valtism commented Nov 26, 2021 •

edited

Loading

gazcn007 left a comment

williaster commented Jan 4, 2022

williaster left a comment

williaster Jan 19, 2022

valtism commented Jan 20, 2022

williaster left a comment

williaster Jan 20, 2022

williaster Jan 20, 2022 •

edited

Loading

williaster Jan 20, 2022

williaster commented Jan 21, 2022

github-actions bot commented Jan 21, 2022

valtism commented Jan 23, 2022

	/** Pass in a custom element as the label to style as you like. Renders inside a <foreignObject> */
	/** Pass in a custom element as the label to style as you like. Renders inside a <foreignObject>. */

Annotation label <foreignobject> render #1383

Annotation label <foreignobject> render #1383

Conversation

valtism commented Nov 26, 2021

🚀 Enhancements

valtism commented Nov 26, 2021 • edited Loading

gazcn007 left a comment

Choose a reason for hiding this comment

williaster commented Jan 4, 2022

williaster left a comment

Choose a reason for hiding this comment

williaster Jan 19, 2022

Choose a reason for hiding this comment

valtism commented Jan 20, 2022

williaster left a comment

Choose a reason for hiding this comment

williaster Jan 20, 2022

Choose a reason for hiding this comment

williaster Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

williaster Jan 20, 2022

Choose a reason for hiding this comment

williaster commented Jan 21, 2022

github-actions bot commented Jan 21, 2022

valtism commented Jan 23, 2022

valtism commented Nov 26, 2021 •

edited

Loading

williaster Jan 20, 2022 •

edited

Loading