feat: enable or disable scatter dot on charts except scatter chart #769

JennChao · 2020-08-27T03:37:38Z

Updates

Allow user to enable or disable scatter dot in charts except scatter chart.

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad · 2020-08-27T14:06:03Z

packages/core/src/components/graphs/scatter-stacked.ts


 export class StackedScatter extends Scatter {
 	type = "scatter-stacked";

 	render(animate: boolean) {
+		const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "scatterDotEnabled");
+		console.log("!!! this.configs.ignoreScatterDisabled: ", this.configs.ignoreScatterDisabled);


console.logs?

theiliad · 2020-08-27T14:07:42Z

packages/core/src/configuration.ts

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
 	axes,
 	timeScale,
 	grid,
+	scatterDotEnabled: true,


probably under the points flag as enabled?

if we move scatterDotEnabled under the points flag as enabled, then only scatter chart can access the config option. However, axis charts, such as line chart and area chart, also need to access this config option as well.

Don't think this'd be the case. Any chart that uses scatter should extend scatter chart and their options should do the same

Sometimes we don't want to have the scatter dot displayed. That's the reason why I setup this scatterDotEnabled option for letting the user enable or disable the scatter dot. And please note that, there will always be scatter dot displayed on scatter charts. Only line chart, area chart, and step chart can have the option to disable scatter dot.

Yes in the screenshot you have above, AreaChart should be extending scatter which means the enabled flag would be available in there

I think I need to figure out what you mean by saying extending scatter chart. Both area chart and scatter chart extend AxisChart and have scatter component in them. That's the reason why we can see the scatter dot. And from my understanding, scatter component will only be newed in AxisChart.

From what I have observed above, I think whether we want to show or not show the scatter dot, Scatter component, should not be configure in scatterChart option. scatterDotEnabled should be specify in axisChart option so all the axis charts with Scatter component can access this config option.

what @theiliad is saying is that the ScatterChartOptions extends the AxisChartOptions meaning that all charts that have Scatter dots will have ScatterChartOptions in addition to the AxisChartOptions.

so if you change ScatterChartOptions like this (charts.ts):

/** * options specific to scatter charts */ export interface ScatterChartOptions extends AxisChartOptions { /** * options for the points */ points?: { /** * sets the radius of the point */ radius: number; fillOpacity?: number; filled?: boolean; enabled?: boolean; }; }

than you can update how you access the config in both scatter-stacked.ts and scatter.ts:
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "points", "enabled");

theiliad · 2020-08-27T14:09:55Z

packages/core/src/charts/scatter.ts

@@ -44,7 +44,9 @@ export class ScatterChart extends AxisChart {
 			new TwoDimensionalAxes(this.model, this.services),
 			new Grid(this.model, this.services),
 			new Ruler(this.model, this.services),
-			new Scatter(this.model, this.services),
+			new Scatter(this.model, this.services, {
+				ignoreScatterDisabled: true


we probably need a more readable name here. If I was looking at this file for the first time I probably wouldn't understand what this flag would do

how about alwaysEnableScatterDot?

theiliad · 2020-08-31T15:18:16Z

packages/core/src/configuration.ts

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
 	axes,
 	timeScale,
 	grid,
+	scatterDotEnabled: true,


Don't think this'd be the case. Any chart that uses scatter should extend scatter chart and their options should do the same

theiliad · 2020-09-01T17:17:41Z

packages/core/src/configuration.ts

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
 	axes,
 	timeScale,
 	grid,
+	scatterDotEnabled: true,


Yes in the screenshot you have above, AreaChart should be extending scatter which means the enabled flag would be available in there

…to scatter-option

theiliad

This flag should really be under points. If a user manually chooses to also disable scatter dots on just a scatter chart, that's just a user error. They will immediately realize that this isn't a great idea and enable the points again. No reason to cause inconsistencies to APIs here

natashadecoste · 2020-09-10T20:49:45Z

packages/core/src/configuration.ts

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
 	axes,
 	timeScale,
 	grid,
+	scatterDotEnabled: true,


what @theiliad is saying is that the ScatterChartOptions extends the AxisChartOptions meaning that all charts that have Scatter dots will have ScatterChartOptions in addition to the AxisChartOptions.

so if you change ScatterChartOptions like this (charts.ts):

/** * options specific to scatter charts */ export interface ScatterChartOptions extends AxisChartOptions { /** * options for the points */ points?: { /** * sets the radius of the point */ radius: number; fillOpacity?: number; filled?: boolean; enabled?: boolean; }; }

than you can update how you access the config in both scatter-stacked.ts and scatter.ts:
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "points", "enabled");

…to scatter-option * 'master' of https://github.com/JennChao/carbon-charts: v0.36.4 Merge pull request carbon-design-system#761 from JennChao/sparkline Merge pull request carbon-design-system#793 from hlyang397/update-initial-zoom-domain fix the defect of label tooltip not showing when using custom tooltip (carbon-design-system#787) v0.36.3 Merge pull request carbon-design-system#785 from sophiiae/skeleton-with-data

…to linearGradient * 'master' of https://github.com/JennChao/carbon-charts: v0.37.0 Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769) feat: create options for tick rotation (carbon-design-system#770)

…to axis-option * 'master' of https://github.com/JennChao/carbon-charts: v0.37.0 Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769) feat: create options for tick rotation (carbon-design-system#770) fix: enable zoom in functionality while axis is disabled

…to ruler-option * 'master' of https://github.com/JennChao/carbon-charts: v0.37.1 Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart v0.37.0 Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769) feat: create options for tick rotation (carbon-design-system#770)

…to tooltip-option * 'master' of https://github.com/JennChao/carbon-charts: v0.37.1 Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart v0.37.0 Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769) feat: create options for tick rotation (carbon-design-system#770) v0.36.4 Merge pull request carbon-design-system#761 from JennChao/sparkline Merge pull request carbon-design-system#793 from hlyang397/update-initial-zoom-domain fix the defect of label tooltip not showing when using custom tooltip (carbon-design-system#787) v0.36.3 Merge pull request carbon-design-system#785 from sophiiae/skeleton-with-data

…arbon-design-system#769) * feat: enable or disable scatter dot on charts except scatter chart * refactor: rename variable * fix: always enable scatter dot for bubble chart * refactor: have enabled option within points

feat: enable or disable scatter dot on charts except scatter chart

e434491

JennChao requested review from natashadecoste, theiliad and a team as code owners August 27, 2020 03:37

JennChao requested review from sophiiae and removed request for a team August 27, 2020 03:37

theiliad requested changes Aug 27, 2020

View reviewed changes

refactor: rename variable

f55700c

JennChao requested a review from theiliad August 28, 2020 09:16

theiliad requested changes Aug 31, 2020

View reviewed changes

JennChao requested a review from theiliad September 1, 2020 10:15

theiliad requested changes Sep 1, 2020

View reviewed changes

JennChao requested a review from theiliad September 2, 2020 09:36

JennChao added 3 commits September 2, 2020 17:56

Merge branch 'master' of https://github.com/JennChao/carbon-charts in…

68f0af7

…to scatter-option

Merge branch 'master' of https://github.com/JennChao/carbon-charts in…

d1aa973

…to scatter-option

fix: always enable scatter dot for bubble chart

77ebcf3

theiliad requested changes Sep 10, 2020

View reviewed changes

natashadecoste suggested changes Sep 10, 2020

View reviewed changes

JennChao requested review from natashadecoste and theiliad September 11, 2020 05:30

refactor: have enabled option within points

a141dfc

theiliad approved these changes Sep 11, 2020

View reviewed changes

natashadecoste approved these changes Sep 11, 2020

View reviewed changes

theiliad added the type: enhancement 💡 label Sep 11, 2020

theiliad merged commit 3131942 into carbon-design-system:master Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable or disable scatter dot on charts except scatter chart #769

feat: enable or disable scatter dot on charts except scatter chart #769

JennChao commented Aug 27, 2020

theiliad Aug 27, 2020

theiliad Aug 27, 2020

JennChao Aug 28, 2020

theiliad Aug 31, 2020

JennChao Sep 1, 2020

theiliad Sep 1, 2020

JennChao Sep 2, 2020

natashadecoste Sep 10, 2020

theiliad Aug 27, 2020

JennChao Aug 28, 2020

theiliad Aug 31, 2020

theiliad Sep 1, 2020

theiliad left a comment

natashadecoste Sep 10, 2020

feat: enable or disable scatter dot on charts except scatter chart #769

feat: enable or disable scatter dot on charts except scatter chart #769

Conversation

JennChao commented Aug 27, 2020

Updates

Demo screenshot or recording

Review checklist (for reviewers only)

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

theiliad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment