Add <Legend> into <LineChart> #14

chenesan · 2019-03-04T10:25:54Z

Purpose

Add <Legend> component, and integrate it into <LineChart>.

Change

graph

Add <Legend> and <LegendGroup>.
Fix ordinal color scale: Make colors.sequential.scheme be ReadonlyArray<string>[] to include multiple color arrays to fix ordinal color scale not map color properly.
Consider legend width in getInnerGraphDimension so graph width won't include legend width.
Fix typescript error when render <HoverLayer> without passing defaultProps (throttleTime, handleHover).

chart

Add <LegendGroup> and related encoding props into <LineChart>.

UI screenshot

nominal color field line chart

ordinal color field line chart

line chart without legend

line chart with custom legend

`orient=left`

`orient=top`

`orient=bottom`

`direction=horizontal`

`direction=vertical`

Risks

None.

Todos

~~Consider legend width/height in getInnerGraphDimension~~.
~~Add basic legend encoding to allow customize legend~~.

chenesan · 2019-03-05T09:22:14Z

docs/charts/LineChart.mdx

@@ -108,12 +110,75 @@ const lineData = [
      y={{ type: 'quantitative', field: 'y' }}
      color={{
        field: "size",
-        type: "quantitative",
+        type: "ordinal",


If it's quantitative the label in legend will be independent to data point value (size) and paint a discrete scale. This should be ordinal so the legend will exactly match size.

chenesan · 2019-03-05T09:25:54Z

docs/charts/LineChart.mdx

+  </div>
+</Playground>
+
+### Multiple lines Without legend


See the screenshot in PR description :-)

chenesan · 2019-03-05T09:26:00Z

docs/charts/LineChart.mdx

      }}
    />
  </div>
 </Playground>

+### Multiple lines with custom legend


See the screenshot in PR description :-)

chenesan · 2019-03-05T09:27:57Z

packages/chart/src/line/LineChart.tsx

  const dimension = useContainerDimension(chartRef);
+  const { width: legendWidth } = useContainerDimension(legendRef);


The legend should be "outside" of the graph. So we have to have a ref on the <LegendGroup>, detect its width and bring it into getInnerGraphDimension to make sure graph will not intersect with legend.

chenesan · 2019-03-05T09:29:28Z

packages/chart/src/line/LineChart.tsx

+      encoding: color,
+      colors: theme.colors,
+    })
+    : null;


Make it return null because we can use non null operator in https://github.com/iCHEF/transcharts/pull/14/files#diff-d2fea036f66e094259cb8577ae881e96R258 . Not sure if there's better way, though.

chenesan · 2019-03-05T09:41:19Z

packages/graph/src/common/types.ts

@@ -99,7 +112,7 @@ export interface Theme {
    /** colors used for nominal data */
    category: ReadonlyArray<string>;
    sequential: {
-      scheme: ReadonlyArray<string>;
+      scheme: ReadonlyArray<ReadonlyArray<string>>;


This adjustment is because scaleOrdinal (we used in getColorScale) will map field value to color one by one from the start of scheme array, which will make mapped color too light when domain size is much smaller than the scheme size. For example when we only have large, small 2 sizes but have 9 colors in scheme, the scaleOrdinal will map this 2 sizes to the 0th and 1st color in the color scheme, which is too light to read it clearly.

To fix this, the scheme should be an array of array of color (which is https://github.com/d3/d3-scale-chromatic#schemeBlues). When configure color scale, just use the color scheme as large as the domain size, so the scale could map color properly.

chenesan · 2019-03-05T09:42:54Z

packages/graph/src/hooks/useContainerDimension.ts

      };
    },
-    [],
+    [containerRef],


The containerRef would be null when attaching ref on legend and be updated after first render. We have to add containerRef to detect the change.

chenesan · 2019-03-05T09:44:00Z

packages/graph/src/hooks/useContainerDimension.ts

    () => {
      // connect the resize observer on mounted
-      resizeObsrRef.current = new resizeObserverPolyfill(debouncedResize);
-      resizeObsrRef.current.observe(containerRef.current!);
+      if (containerRef.current) {


Because the containerRef.current could be null when detecting <LegendGroup> width, we have to check it here.

chenesan · 2019-03-05T09:44:59Z

packages/graph/src/hooks/useContainerDimension.ts

@@ -64,18 +64,22 @@ export function useContainerDimension(
    [],
  );

-  useEffect(
+  useLayoutEffect(


I'm not that sure. But it seems that we should "wait" for the layout change (element attached) and bind the observer.

Can you elaborate on why you're choosing useLayoutEffect over useEffect?
I believe the later is preferred by the official docs of React Hooks.
Ref: https://reactjs.org/docs/hooks-reference.html#uselayouteffect

After some thinking I agree that this should be useEffect. What I want is I'd like to make sure when the effect is running the container element has been in DOM. But since the containerRef would been changed from null to the html element before the effect running, useEffect should be better one.

I think after you remove the following statements in you could use useEffect without any issue:

if (graphWidth <= 0 || graphHeight <= 0) { return null; }

chenesan · 2019-03-05T09:51:50Z

packages/graph/src/layers/HoverLayer.tsx

-  collisionComponents,
-  throttleTime = 180,
-}) => {
+export const HoverLayer = (props: HoverLayerProps) => {


The point of this fix is, even we have default argument (handleHover and throttleTime) in HoverLayer, we still get typescript error when rendering it without passing them:

/* oops */ <HoverLayer setHoveredPosAndIndex={...} clearHovering={...} collisionPoints={...} />

It's because FunctionComponent<HoverLayerProps> type won't consider the default arguments and regard the handleHover and throttleTime are required. So I fixed them with the indication in react-typescript-cheatsheet.

(This won't make docz break so I didn't find out this until I try to build it today 😢 😭 💣 . I think we should fix the difference between docz build and lerna run prepublish and add CI check as soon as possible.)

chenesan · 2019-03-05T09:53:23Z

packages/graph/src/legend/Legend.tsx

+const SCALE_TYPE_TO_LEGEND = {
+  ordinal: LegendOrdinal,
+  sequential: LegendLinear,
+};


Currently we only accept two type of scale for legend. I think it's fine and we can have more if needed.

chenesan · 2019-03-05T09:55:04Z

packages/graph/src/legend/Legend.tsx

+    <div>
+      <div>{title}</div>
+      <LegendComponent scale={scale}>
+        {render}


We pass the render as children render prop in vx Legend. See the vx legend gallery and vx legend doc

chenesan · 2019-03-05T09:58:07Z

packages/graph/src/legend/LegendGroup.tsx

+ * LegendGroup will return corresponding legend.
+ *
+ */
+export const LegendGroup = forwardRef((props: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {


The reason to have LegendGroup is because

We may have multiple legends in one place if there are multiple scales (color / shape / size...)

I'd like to handle encoding / legend config outside our own Legend.

The second reason is fair enough, but I have some hard time imaging the usage of first reason.
Can you picture a rough example of how a <LegendGroup> can benefit multiple legends?

Something like this:

chenesan · 2019-03-05T09:58:35Z

packages/graph/src/themes/index.tsx

@@ -15,7 +15,7 @@ export const themes = {
    colors: {
      category: schemeCategory10,
      sequential: {
-        scheme: schemeBlues[9],
+        scheme: schemeBlues,


See https://github.com/iCHEF/transcharts/pull/14/files#r262417031

chenesan · 2019-03-05T09:59:44Z

packages/graph/src/utils/getColorScale.ts

@@ -42,13 +42,14 @@ const getColorScaleSetting = ({
      };
    }
    case 'ordinal': {
-      const domain = map(data, field).sort((a, b) => Number(a) - Number(b));
-      const scale = scaleOrdinal(colors.sequential.scheme).domain(domain);
+      const domain = sortedUniq(map(data, field).sort((a, b) => Number(a) - Number(b)));


We have to make sure the element is unique or the scaleOrdinal will treat same element in domain as different value and map it incorrectly.

Nice change 👍. I think you could put this note into the comment.

zhusee2 · 2019-03-06T08:45:39Z

docs/charts/LineChart.mdx

+      data={multiLinesData}
+      x={{ type: 'quantitative', field: 'x' }}
+      y={{ type: 'quantitative', field: 'y' }}
+      color={{


As a user, I think the color prop has became too complex here.
It obviously contains much more than its literal name color

Hmmm... I agree that it's becoming larger. And that's because I tried to use the same (not fully equal to) schema as data encoding in vega-lite. With this we can have (mostly) same properties for different visual feature (color / opacity / size...) to make the api more consistent. And the legend is also included in encoding object.

Also See #5 for some information about this.

I think that leaving vx-legend to handle the custom legend is a nice design here. Legends are shown by default if the color scale is provided and users are not required to customize the legends.

We may adopt similar approaches to let users customize the tooltip so that our API could be more consistent.

For users familiar with vx it might be great as they can share existing experiences.
I was thinking it could be even better if we can design a more easy-to-use API, especially we are creating a new library.

But we can talk about that later, there's no hurry to block this PR.

I'm not that sure in the legend props of color (That would be another problem that if legend should be under color) if we should

have the same props (except hide and render) as vx-legend (even just pass the remaining properties of legend down to <Legend> of vx-legend)

have the same props as vega-lite legend config.

Personally I may prefer to (1) because it's easier to implement and also intuitive to vx user.

hsunpei

Great integration with the vx-legend library and elegant implementation of the <LegendGroup> ✨

I think properties like direction if the legend and it's orient could be furtherly added since charts may not share the same placement of the legend when the layout is different.

It would be nice if a separate document about the usages of <Legend> in docz be included to let users know about how to customize the legends 😃 .

hsunpei · 2019-03-07T07:21:28Z

packages/graph/src/legend/Legend.tsx

+  scaleType: string;
+  scale: (val: any) => string;
+  title: string;
+  render?: FunctionComponent | null | undefined;


Should the type of render be: (labels: ReadonlyArray<object>) => React.ReactNode ?

hsunpei · 2019-03-07T07:27:02Z

packages/graph/src/legend/LegendGroup.tsx

+ * LegendGroup will return corresponding legend.
+ *
+ */
+export const LegendGroup = forwardRef((props: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {


I think you may destruct the props here

Suggested change

export const LegendGroup = forwardRef((props: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {

export const LegendGroup = forwardRef(({ color }: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {

hsunpei · 2019-03-07T07:29:31Z

packages/graph/src/legend/LegendGroup.tsx

+  const { color } = props;
+  if (!color || (color.legend && color.legend.hide)) {
+    return null;
+  }


Nice design decision that takes multiple scales into account 👍 .
Maybe TODO: ... comments about supporting other scales could be added here.

hsunpei · 2019-03-07T07:40:21Z

packages/graph/src/utils/getColorScale.ts

@@ -42,13 +42,14 @@ const getColorScaleSetting = ({
      };
    }
    case 'ordinal': {
-      const domain = map(data, field).sort((a, b) => Number(a) - Number(b));
-      const scale = scaleOrdinal(colors.sequential.scheme).domain(domain);
+      const domain = sortedUniq(map(data, field).sort((a, b) => Number(a) - Number(b)));


Nice change 👍. I think you could put this note into the comment.

hsunpei · 2019-03-07T08:29:42Z

docs/charts/LineChart.mdx

+      data={multiLinesData}
+      x={{ type: 'quantitative', field: 'x' }}
+      y={{ type: 'quantitative', field: 'y' }}
+      color={{


I think that leaving vx-legend to handle the custom legend is a nice design here. Legends are shown by default if the color scale is provided and users are not required to customize the legends.

We may adopt similar approaches to let users customize the tooltip so that our API could be more consistent.

hsunpei · 2019-03-07T08:49:38Z

packages/graph/src/legend/LegendGroup.tsx

+import { Legend } from './Legend';
+
+interface LegendGroupProps {
+  color?: ColorScale & { legend?: LegendConfig };


Some props for configuring the placement like the direction and orient (see Legend of vx-lite) could be further included, since these properties are supported in vx-legend and users may want to apply different settings of direction and orient in different charts.

hsunpei · 2019-03-07T08:59:12Z

packages/graph/src/hooks/useContainerDimension.ts

@@ -64,18 +64,22 @@ export function useContainerDimension(
    [],
  );

-  useEffect(
+  useLayoutEffect(


I think after you remove the following statements in you could use useEffect without any issue:

if (graphWidth <= 0 || graphHeight <= 0) { return null; }

chenesan · 2019-03-11T04:30:23Z

@garfieldduck @zhusee2 I just added orient and direction in legend config and create independent legend document page. The screenshot has been updated in PR description. It's ok to review this again now :)

hsunpei

Excellent implementations of legends and nice documentation. 💯🚀

zhusee2

LGTM as a batch

zhusee2 · 2019-03-12T02:23:57Z

packages/chart/src/utils/getInnerGraphDimensionAndMargin.ts

+    default: {
+      break;
+    }
+  }


(optional comment)
I believe it's possible to save some duplicated codes by making your switch cases fall-through, and maybe also by checking outer values outside the switch block:

let graphWidth = (outerWidth <= 0) ? DEFAULT_LENGTH : undefined; let graphHeight = (outerHeight <= 0) ? DEFAULT_LENGTH : undefined; switch (orient) { case LEFT: case RIGHT: graphWidth = graphWidth || value; graphHeight = graphHeight || value; break; case TOP: case BOTTOM: // also do the calc break; }

chenesan self-assigned this Mar 4, 2019

chenesan force-pushed the feature/legend branch 3 times, most recently from 6f52591 to c9502bb Compare March 5, 2019 09:15

chenesan commented Mar 5, 2019

View reviewed changes

chenesan force-pushed the feature/legend branch 3 times, most recently from 6205b24 to 757fd90 Compare March 5, 2019 10:07

chenesan changed the title ~~[WIP] Add <Legend> into <LineChart>~~ Add <Legend> into <LineChart> Mar 5, 2019

chenesan changed the base branch from feature/color-scale to develop March 5, 2019 10:08

chenesan requested review from hsunpei, tz5514 and zhusee2 March 5, 2019 10:09

zhusee2 reviewed Mar 6, 2019

View reviewed changes

hsunpei reviewed Mar 7, 2019

View reviewed changes

chenesan mentioned this pull request Mar 7, 2019

Setup unit test with jest, ts-jest and enzyme #17

Merged

chenesan force-pushed the feature/legend branch 3 times, most recently from bd37a3c to 2f95e78 Compare March 11, 2019 04:28

chenesan force-pushed the feature/legend branch from 2f95e78 to f31a666 Compare March 11, 2019 06:15

hsunpei approved these changes Mar 11, 2019

View reviewed changes

zhusee2 approved these changes Mar 11, 2019

View reviewed changes

zhusee2 reviewed Mar 12, 2019

View reviewed changes

chenesan added 16 commits March 12, 2019 14:23

Add @vx/legend dependency and types

6f55c01

Add Legend component

4be55ba

Add legend in chart

f2d62a2

Fix ordinal scale color

ebc7e2a

Add LegendGroup component

0deecd9

Fix linter issue of graph package

62ddb55

Apply LegendGroup in LineChart

c4bf180

Fix HoverLayer defaultProps

ec642b6

Fix useContainerDimension with watching containerRef

7504db7

Consider legend width in LineChart

32ef479

Update Legend usage in LineChart document

87a8184

Update yarn.lock

8e8a85b

Add direction attribute in legend

49a78cf

Add orient attribute in legend config

05466dd

Add independent legend doc

5706e35

Update CHANGELOG

717ba5f

chenesan force-pushed the feature/legend branch from f31a666 to 717ba5f Compare March 12, 2019 06:47

chenesan merged commit 12f0ab7 into develop Mar 12, 2019

chenesan deleted the feature/legend branch March 12, 2019 06:48

chenesan mentioned this pull request Mar 18, 2019

Release/0.0.2 #26

Merged

		const dimension = useContainerDimension(chartRef);
		const { width: legendWidth } = useContainerDimension(legendRef);

	export const LegendGroup = forwardRef((props: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {
	export const LegendGroup = forwardRef(({ color }: LegendGroupProps, ref: RefObject<HTMLDivElement>) => {

Add <Legend> into <LineChart> #14

Add <Legend> into <LineChart> #14

Conversation

chenesan commented Mar 4, 2019 • edited Loading

Purpose

Change

graph

chart

UI screenshot

nominal color field line chart

ordinal color field line chart

line chart without legend

line chart with custom legend

orient=left

orient=top

orient=bottom

direction=horizontal

direction=vertical

Risks

Todos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenesan Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenesan Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhusee2 Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenesan Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

hsunpei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenesan commented Mar 11, 2019 • edited Loading

hsunpei left a comment

Choose a reason for hiding this comment

zhusee2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenesan commented Mar 4, 2019 •

edited

Loading

`orient=left`

`orient=top`

`orient=bottom`

`direction=horizontal`

`direction=vertical`

chenesan Mar 5, 2019 •

edited

Loading

chenesan Mar 5, 2019 •

edited

Loading

zhusee2 Mar 6, 2019 •

edited

Loading

chenesan Mar 8, 2019 •

edited

Loading

chenesan commented Mar 11, 2019 •

edited

Loading