feat: enable chart grid enable option #761

JennChao · 2020-08-25T06:46:58Z

Updates

Sometimes user might not want to see chart grid. This PR provide the option of showing or not showing chart grid.

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad

we should probably allow individual grid enablement flags...

e.g. x is active but y is disabled

theiliad · 2020-08-25T15:45:40Z

packages/core/src/components/axes/grid.ts

@@ -15,7 +15,7 @@ export class Grid extends Component {
 		const gridEnable = Tools.getProperty(


Suggested change

const gridEnable = Tools.getProperty(

const isGridEnabled = Tools.getProperty(

theiliad · 2020-08-25T15:45:54Z

packages/core/src/components/axes/grid.ts

@@ -15,7 +15,7 @@ export class Grid extends Component {
 		const gridEnable = Tools.getProperty(
 			this.model.getOptions(),
 			"grid",
-			"enable"
+			"enabled"
 		);
 		if (!gridEnable) {


Suggested change

if (!gridEnable) {

if (!gridEnable) {

theiliad · 2020-08-25T15:46:24Z

packages/core/src/configuration.ts

@@ -62,7 +62,7 @@ const legend: LegendOptions = {
 */
 export const grid: GridOptions = {
 	// set enable to false will not draw grid and stroke of grid backdrop
-	enable: true,
+	enabled: false,


grid cannot be disabled by default

theiliad · 2020-08-25T15:49:42Z

packages/core/src/styles/components/_grid.scss

+		} @else {
+			fill: $ui-background;
+		}
+	}


why do we need this extra classname?

The reason why we need chart-grid-backdrop-no-stroke classname is because we don't want to apply stroke around the chart when there is no grid line.

…to sparkline

JennChao · 2020-08-26T02:57:35Z

we should probably allow individual grid enablement flags...

e.g. x is active but y is disabled

x grid and y grid can now be enabled or disabled separately

natashadecoste · 2020-09-01T00:41:28Z

packages/core/src/configuration.ts

+	// set enable to false will not draw grid and stroke of grid backdrop
+	xGridEnabled: true,
+	yGridEnabled: true,
+


i think this should go within the x and y configurations like this

grid:{ x: { enabled: true, numberOfTicks: 15 }, y: { enabled: true, numberOfTicks: 15 } }

or maybe "visible" is a better word... since its still enabled/functional

Based on @theiliad 's input. I will use "enabled".

theiliad

Hi 👋

Please include some more demos and screenshots so we can see this in action when reviewing, thanks.

theiliad · 2020-09-01T17:32:34Z

packages/core/src/interfaces/components.ts

@@ -85,9 +85,11 @@ export interface ThresholdOptions {

 export interface GridOptions {
 	y?: {
+		yGridEnabled?: boolean;


again, we always will want to keep things consistent by using enabled

theiliad · 2020-09-01T17:33:48Z

packages/core/src/components/axes/ruler.ts

-			"rect.chart-grid-backdrop"
+			this.isXGridEnabled || this.isYGridEnabled
+				? "rect.chart-grid-backdrop.stroke"
+				: "rect.chart-grid-backdrop"


seems like stroke should be a separate configurable no?

A chart with grid but without stroke looks kind of weird. So I guess we would always want to add stroke when there is grid being enabled. And when there is no grid being enabled, we can let the user setup the option to disable stroke? What do you think?
With stroke.

Without stroke.

actually I think stroke might be something we'd always want to keep unless overriden

@jeanservaas could you please chime in?

Hi @theiliad and @jeanservaas,
There is the situation when we don't want to show the stroke.

…to sparkline

theiliad · 2020-09-03T14:42:30Z

packages/core/src/styles/components/_grid.scss

@@ -7,6 +7,8 @@
 		} @else {
 			fill: $ui-background;
 		}
+	}
+	rect.stroke {


Suggested change

rect.stroke {

rect.chart-grid-backdrop.stroked {

theiliad · 2020-09-03T14:42:53Z

packages/core/src/components/axes/ruler.ts

@@ -231,7 +243,9 @@ export class Ruler extends Component {
 		this.backdrop = DOMUtils.appendOrSelect(svg, "svg.chart-grid-backdrop");
 		const backdropRect = DOMUtils.appendOrSelect(
 			this.backdrop,
-			"rect.chart-grid-backdrop"
+			this.isXGridEnabled || this.isYGridEnabled
+				? "rect.chart-grid-backdrop.stroke"


Suggested change

? "rect.chart-grid-backdrop.stroke"

? "rect.chart-grid-backdrop.stroked"

again my feedback here still applies. seems like Ruler doesn't need to style the backdrop, just needs to grab it.

this piece seems to be in there by mistake from our previous commits.

Are you able to remove all the styling pieces for the backdrop from Ruler and test whether things still work? We should remove any of it that doesn't break charts.

Removing the code for styling backdrop in Ruler component has already been done in #765

theiliad · 2020-09-03T14:43:06Z

packages/core/src/components/axes/grid.ts

@@ -197,7 +218,9 @@ export class Grid extends Component {
 		this.backdrop = DOMUtils.appendOrSelect(svg, "svg.chart-grid-backdrop");
 		const backdropRect = DOMUtils.appendOrSelect(
 			this.backdrop,
-			"rect.chart-grid-backdrop"
+			isXGridEnabled || isYGridEnabled
+				? "rect.chart-grid-backdrop.stroke"


Suggested change

? "rect.chart-grid-backdrop.stroke"

? "rect.chart-grid-backdrop.stroked"

…to ruler-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 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.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 axis-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 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

* feat: enable chart grid enable option * refactor: rename grid option * refactor: rename gridEnabled to isGridEnabled * fix: enable grid as default setting * feat: enable or disable x and y grid separately * refactor: interface modification for xGridEnabled and yGridEnabled * refactor: rename grid option * refactor: rename class Co-authored-by: Fei Z <[email protected]>

feat: enable chart grid enable option

c00d4f5

JennChao requested review from natashadecoste, theiliad and a team as code owners August 25, 2020 06:46

JennChao requested review from zvonimirfras and removed request for a team August 25, 2020 06:46

refactor: rename grid option

426d2a9

theiliad requested changes Aug 25, 2020

View reviewed changes

JennChao added 4 commits August 26, 2020 09:55

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

ff71f3a

…to sparkline

refactor: rename gridEnabled to isGridEnabled

9672840

fix: enable grid as default setting

4de05e1

feat: enable or disable x and y grid separately

965513b

JennChao requested a review from theiliad August 26, 2020 02:57

natashadecoste suggested changes Sep 1, 2020

View reviewed changes

refactor: interface modification for xGridEnabled and yGridEnabled

a5ea445

theiliad requested changes Sep 1, 2020

View reviewed changes

JennChao added 2 commits September 2, 2020 15:36

Merge branch 'master' into branch 'sparkline'

774f6a4

refactor: rename grid option

ec9212a

JennChao requested review from natashadecoste and theiliad September 2, 2020 08:14

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

4d97cdf

…to sparkline

theiliad requested changes Sep 3, 2020

View reviewed changes

refactor: rename class

068846f

JennChao requested a review from theiliad September 7, 2020 08:10

Merge branch 'master' into sparkline

d6a4177

natashadecoste approved these changes Sep 10, 2020

View reviewed changes

theiliad approved these changes Sep 10, 2020

View reviewed changes

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

theiliad merged commit dc4087b into carbon-design-system:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable chart grid enable option #761

feat: enable chart grid enable option #761

JennChao commented Aug 25, 2020

theiliad left a comment

theiliad Aug 25, 2020

theiliad Aug 25, 2020

theiliad Aug 25, 2020

theiliad Aug 25, 2020

JennChao Aug 26, 2020

JennChao commented Aug 26, 2020

natashadecoste Sep 1, 2020 •

edited

Loading

JennChao Sep 2, 2020

theiliad left a comment

theiliad Sep 1, 2020

theiliad Sep 1, 2020

JennChao Sep 2, 2020

theiliad Sep 2, 2020

JennChao Sep 3, 2020

theiliad Sep 3, 2020

theiliad Sep 3, 2020

theiliad Sep 3, 2020

JennChao Sep 7, 2020

theiliad Sep 3, 2020

		@@ -15,7 +15,7 @@ export class Grid extends Component {
		const gridEnable = Tools.getProperty(

	const gridEnable = Tools.getProperty(
	const isGridEnabled = Tools.getProperty(

	? "rect.chart-grid-backdrop.stroke"
	? "rect.chart-grid-backdrop.stroked"

feat: enable chart grid enable option #761

feat: enable chart grid enable option #761

Conversation

JennChao commented Aug 25, 2020

Updates

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad 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

JennChao commented Aug 26, 2020

natashadecoste Sep 1, 2020 • edited Loading

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

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

natashadecoste Sep 1, 2020 •

edited

Loading