Add getColorScale to enable displaying multiple lines in LineChart #11

chenesan · 2019-02-26T10:38:51Z

Purpose

Add getColorScale. Leverage this color scale to paint multiple lines in LineChart.

Change

Add getColorScale in graph package.
Paint multiple lines in line chart (given color encoding prop)

UI screenshot

nominal field as color

quantitative field as color

Risk

None.

TODO

[ ] Handle quantitative scaling properly so the lightest line in example in "quantitative field as color" could be clearer.

chenesan · 2019-02-26T10:41:30Z

packages/chart/src/line/LineChart.tsx

-            fill={color}
-          />
-        ));
+        const graphGroup = dataGroup.map(


After adding color encoding the render function is larger and larger (more than 100~ lines now!). I'm not sure if there are better way to arrange them. Comment / idea are welcome =D.

Hmmm...it is indeed. But I cannot think of a way to deal with this neither. Maybe the <DataLine> could be extracted.

chenesan · 2019-02-26T10:42:19Z

packages/chart/src/line/LineChart.tsx

+        const fieldsToGroupBy: string[] = [color]
+          .filter(encoding => !!encoding)
+          .map(encoding => encoding.field);
+        const sortedData = data.sort((rowA, rowB) => xSelector.getOriginalVal(rowA) - xSelector.getOriginalVal(rowB));


I found that if we don't sort it the lines will not be a path always from left to right..

chenesan · 2019-02-26T10:42:49Z

packages/graph/src/common/types.ts

@@ -88,6 +93,10 @@ export interface Theme {
  colors: {
    /** colors used for nominal data */
    category: ReadonlyArray<string>;
+    sequential: {


See d3-scale-chromatic

hsunpei

Looks wonderful. Nice handlings with the colors scales and the data grouping functions 🎉✨
Some documentation may be further included.

hsunpei · 2019-02-27T10:59:11Z

packages/graph/src/common/types.ts

@@ -29,7 +29,7 @@ export interface Scale {
  /**
   * Range of input channel
   */
-  range: [any, any];
+  range?: ReadonlyArray<any>;


Nice use of ReadonlyArray 👍

hsunpei · 2019-02-27T11:04:34Z

packages/graph/src/utils/getColorScale.ts

+
+import { getRecordFieldSelector } from './getRecordFieldSelector';
+
+interface GetColorScaleArgs {


Could it be named as ColorScaleArgs? Get as the initials sound like a function (though it is capitalized).

hsunpei · 2019-02-27T11:27:18Z

packages/graph/src/utils/getColorScale.ts

+          };
+        },
+        {},
+      ));


How about using the map function of lodash?

const domain = _.map(data, 'field');

hsunpei · 2019-02-27T11:28:05Z

packages/graph/src/utils/getColorScale.ts

+      };
+    }
+    default: {
+      throw Error('Invalid color encoding type.');


Nice error message 👍

hsunpei · 2019-02-27T11:31:24Z

packages/graph/src/utils/getColorScale.ts

+    case 'quantitative': {
+      const values = data.map(row => Number(row[field]));
+      const min = Math.min(...values);
+      const max = Math.max(...values);


How about utilizing the d3-array to handle calculations of the range?

Good idea :)

hsunpei · 2019-02-27T12:09:15Z

packages/graph/src/utils/getDataGroup.ts

+ *   [{ type: 'a', color: 'green' }],
+ *   [{ type: 'b', color: 'green' }],
+ *   [{ type: 'a', color: 'red' }],
+ *   [{ type: 'b', color: 'red' }],


Very nice documentation 👍
I was wondering if this example could be extended with another field (such as value: 1) so that the example could be easier to be understood at first glance.

hsunpei · 2019-02-27T12:10:10Z

packages/graph/src/utils/getDataGroup.ts

+ *   [{ type: 'b', color: 'red' }],
+ * ]
+ */
+export function getDataGroup(data: object[], fields: string[]): object[][] {


Could this be renamed as getGroupedDataByFields?

hsunpei · 2019-02-27T12:13:42Z

packages/chart/src/line/LineChart.tsx

+        const defaultColor = theme.colors.category[0];
+        const getColor = colorScale ?
+          colorScale.selector.getScaledVal :
+          () => defaultColor;


You may follow the coding style of ternaries from airbnb:

const getColor = colorScale ? colorScale.selector.getScaledVal : () => defaultColor;

hsunpei · 2019-02-27T12:16:08Z

packages/chart/src/line/LineChart.tsx

+          () => defaultColor;
+        const fieldsToGroupBy: string[] = [color]
+          .filter(encoding => !!encoding)
+          .map(encoding => encoding!.field);


Maybe this could be extracted as a function in utils

Thanks for suggestion! I would extract a getDataGroupByEncodings since the fieldsToGroupBy is only used in building dataGroup.

hsunpei · 2019-02-27T12:18:09Z

packages/chart/src/line/LineChart.tsx

-            fill={color}
-          />
-        ));
+        const graphGroup = dataGroup.map(


Hmmm...it is indeed. But I cannot think of a way to deal with this neither. Maybe the <DataLine> could be extracted.

chenesan · 2019-03-04T07:21:59Z

@garfieldduck Thanks for review! I have fix them and you can review again now :-)

chenesan · 2019-03-04T07:23:27Z

packages/graph/src/utils/getColorScale.ts

+  data: object[];
+}
+
+function getNumericDomain(values: number[]): [number, number] {


d3Extent return [number, number] | [undefined | undefined] so I have to filter out the undefined result in getNumericDomain

zhusee2 · 2019-03-04T08:36:13Z

packages/chart/src/line/LineChart.tsx

@@ -69,10 +76,43 @@ const HoveringIndicator: FunctionComponent<{
  );
 };

+const DataLine: React.FC<{


I believe I saw this usage instead in #12? I'm not sure which one is correct though.

Suggested change

const DataLine: React.FC<{

const DataLine: React.FunctionComponent<{

The source code of react type definition:

type SFC = FunctionComponent; /** * @deprecated as of recent React versions, function components can no * longer be considered 'stateless'. Please use `FunctionComponent` instead. * * @see [React Hooks](https://reactjs.org/docs/hooks-intro.html) */ type StatelessComponent = FunctionComponent; type FC = FunctionComponent; interface FunctionComponent { (props: P & { children?: ReactNode }, context?: any): ReactElement<any> | null; propTypes?: WeakValidationMap; contextTypes?: ValidationMap<any>; defaultProps?: Partial; displayName?: string; }

So the FC and FunctionComponent should be the same thing. But indeed we had FunctionComponent in previous PR so I will make it consistent with that.

Also I read an article said that you shouldn't use FunctionComponent to type your function al component because they

define additional props like children and contextType, which may not be supported by your component.

break defaultProps resolution in typescript 3.1 (I'm stilly not fully understanding what that means in the article)

For now, I'm not sure if it's really that bad to have FunctionComponent type so I would just keep FunctionComponent here. Once it's clear that we should not use FC or FunctionComponent I may make a fix for this.

zhusee2 · 2019-03-04T08:41:59Z

packages/chart/src/line/LineChart.tsx

  /** Width of the collision detection rectangle */
-  const color = theme.colors.category[0];
+  const bandWidth = graphWidth / (data.length - 1);
+  const colorScale = typeof color !== 'undefined' && getColorScale({


I'll suggest to wrap this expression to make it more prominent.

Suggested change

const colorScale = typeof color !== 'undefined' && getColorScale({

const colorScale = (typeof color !== 'undefined') && getColorScale({

zhusee2 · 2019-03-04T08:55:18Z

packages/graph/src/utils/getColorScale.ts

+  return [
+    typeof extent[0] === 'undefined' ? 0 : extent[0],
+    typeof extent[1] === 'undefined' ? 0 : extent[1],
+  ];


Just some syntax sugar:

const [extentMin = 0, extentMax = 0] = d3.extent(values); return [extentMin, extentMax];

zhusee2

Looks nice to me in general

hsunpei

Great work! Looks fantastic to me 💯 ✨

hsunpei · 2019-03-04T09:50:28Z

packages/chart/src/line/LineChart.tsx

+      fill={color}
+    />
+  ));
+  return (<>


<> may be moved into a new line.

yeah nice catch
+1 with this

chenesan requested review from hsunpei, tz5514 and zhusee2 February 26, 2019 10:38

chenesan self-assigned this Feb 26, 2019

chenesan commented Feb 26, 2019

View reviewed changes

hsunpei reviewed Feb 27, 2019

View reviewed changes

chenesan force-pushed the refactor/axis-and-hover-hook branch from 1102a4b to 705a2af Compare March 4, 2019 03:20

chenesan added 21 commits March 4, 2019 14:11

Add sequential colors in theme

6dab9bb

Make getRecordFieldSelector accept all Scale argument

d760dd0

Add getColorScale

a4d8664

Add multiple color charts in LineChart

ba64566

Add color scale on TooltipLayer and HoverIndicator

2417573

Refactor dataGroup logic

5d911a9

Extract DataLine component

3a329c5

Fix linter issue of TooltipLayer

415c7e0

Fix linter issue of getColorScale

43bec01

Fix linter issue of getDataGroup

b5b09db

Fix linter issue of themes

ef76ce8

Fix sorting issue of DataGroup

5a8b112

Add quantitative example for multi lines

77245f7

Fix undefined type error of LineChart

321eb57

Rename GetColorScaleArgs as ColorScaleArgs

ccebf77

Use d3-array extent in getColorScale

dfed6f6

Fix type definition of getColorScale args

b395275

Add comment on getColorScale

d7eadb1

Rename getDataGroup as getDataGroupByFields

4e47016

Extract getDataGroupByEncodings

f2bfe00

Simplify getColrScaleSetting with lodash.map

d81d659

Fix documentation of getdataGroupByEncodings

5f7f4e5

chenesan force-pushed the feature/color-scale branch from 1bc4867 to 5f7f4e5 Compare March 4, 2019 07:20

chenesan commented Mar 4, 2019

View reviewed changes

chenesan changed the base branch from refactor/axis-and-hover-hook to develop March 4, 2019 07:32

zhusee2 reviewed Mar 4, 2019

View reviewed changes

zhusee2 approved these changes Mar 4, 2019

View reviewed changes

hsunpei approved these changes Mar 4, 2019

View reviewed changes

chenesan added 4 commits March 5, 2019 11:24

Simplify getNumericDomain logic

57bad27

Fix LineChart.tsx formatting

c804601

Update CHANGELOG.md

805c85f

Merge branch 'develop' into feature/color-scale

c60681d

chenesan merged commit 10eddb3 into develop Mar 5, 2019

chenesan mentioned this pull request Mar 18, 2019

Release/0.0.2 #26

Merged

zhusee2 deleted the feature/color-scale branch September 9, 2019 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getColorScale to enable displaying multiple lines in LineChart #11

Add getColorScale to enable displaying multiple lines in LineChart #11

chenesan commented Feb 26, 2019

chenesan Feb 26, 2019

hsunpei Feb 27, 2019

chenesan Feb 26, 2019

chenesan Feb 26, 2019

hsunpei left a comment

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

chenesan Mar 4, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

hsunpei Feb 27, 2019

chenesan Mar 4, 2019

hsunpei Feb 27, 2019

chenesan commented Mar 4, 2019

chenesan Mar 4, 2019

zhusee2 Mar 4, 2019 •

edited

Loading

chenesan Mar 5, 2019

zhusee2 Mar 4, 2019

zhusee2 Mar 4, 2019 •

edited

Loading

zhusee2 left a comment

hsunpei left a comment

hsunpei Mar 4, 2019

zhusee2 Mar 5, 2019


		import { getRecordFieldSelector } from './getRecordFieldSelector';

		interface GetColorScaleArgs {

	const DataLine: React.FC<{
	const DataLine: React.FunctionComponent<{

	const colorScale = typeof color !== 'undefined' && getColorScale({
	const colorScale = (typeof color !== 'undefined') && getColorScale({

Add getColorScale to enable displaying multiple lines in LineChart #11

Add getColorScale to enable displaying multiple lines in LineChart #11

Conversation

chenesan commented Feb 26, 2019

Purpose

Change

UI screenshot

Risk

TODO

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

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

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 4, 2019

Choose a reason for hiding this comment

zhusee2 Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhusee2 Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

zhusee2 left a comment

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

zhusee2 Mar 4, 2019 •

edited

Loading

zhusee2 Mar 4, 2019 •

edited

Loading