feat: enable or disable ruler #765

JennChao · 2020-08-26T07:18:21Z

Updates

User can enable or disable chart ruler.
Another PR or fixing tooltip not showing up when ruler is not applied has already been created. #764

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad

wouldn't this PR need to also add some code in ruler to disable itself if it's been configured to be disabled?

Also @jeanservaas do we want to allow disabling the ruler?

JennChao · 2020-08-27T02:12:57Z

wouldn't this PR need to also add some code in ruler to disable itself if it's been configured to be disabled?

Also @jeanservaas do we want to allow disabling the ruler?

@theiliad
By saying disable ruler itself, do you mean not pushing Ruler component into graphFrameComponents array?

I actually suggest just have the following code in Ruler component. Then we don't need to have duplicated code for modifying all the chart components.

And I think we should allowed the user to configures for disabling the ruler when needed. Here is one example of not ruler in the chart. In this example, all we need is a simple sparkline.

theiliad · 2020-08-27T13:53:28Z

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

@@ -24,10 +24,17 @@ export class Ruler extends Component {
 		domainValue: number;
 		originalData: any;
 	}[];
+	isRulerEnabled =  Tools.getProperty(


this should go inside render, that way it's value will update with future renders

theiliad · 2020-08-27T13:53:40Z

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


 	render() {
 		this.drawBackdrop();
-		this.addBackdropEventListeners();
+		if (this.isRulerEnabled) {


what if ruler was enabled and now isn't?

backdrop event listener will be remove first and then be created is if needed

theiliad · 2020-08-27T13:54:05Z

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

+		this.model.getOptions(),
+		"ruler",
+		"enabled"
+	);

 	render() {
 		this.drawBackdrop();


it seems like drawBackdrop is repeated in ruler & grid. Would ruler fully function without it?

ruler needs backdrop since the event listener is applied on backdrop.

I get that ruler is dependant on backdrop, however I'm wondering if these lines are needed inside ruler.ts

Actually, I am not sure about it. These lines of code have existed even before I added the ruler option.

Yes I'm just asking whether removing them on your end still makes everything work?

As what I can see do far, removing code within line 253 to line 261 won't break any thing on my end.

Codes within line 253 to line 261 have already been removed.

theiliad · 2020-08-31T15:06:36Z

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

+		this.model.getOptions(),
+		"ruler",
+		"enabled"
+	);

 	render() {
 		this.drawBackdrop();


I get that ruler is dependant on backdrop, however I'm wondering if these lines are needed inside ruler.ts

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

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

+		this.model.getOptions(),
+		"ruler",
+		"enabled"
+	);

 	render() {
 		this.drawBackdrop();


Yes I'm just asking whether removing them on your end still makes everything work?

theiliad · 2020-09-01T17:29:20Z

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

+
+		if (isRulerEnabled) {
+			this.addBackdropEventListeners();
+		}


so we remove & add event listeners every time? This doesn't sound healthy, there should be a better way to only remove or add when we need to

Since d3 doesn't have an api for checking whether a specific eventListener is already existed or not. Removing and then adding event listeners when needed is what we can do for now.

again this is the same situation as the other PR, we just need a boolean here to keep track of event listeners

A flag, isEventListenerAdded, has been added.

…to ruler-option

theiliad · 2020-09-10T18:45:07Z

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

+
+		if (isRulerEnabled) {
+			this.addBackdropEventListeners();
+		}


again this is the same situation as the other PR, we just need a boolean here to keep track of event listeners

…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

theiliad · 2020-09-14T14:35:38Z

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

+			this.isEventListenerAdded = true;
+			this.addBackdropEventListeners();
+		} else if (!isRulerEnabled && this.isEventListenerAdded) {
+			this.isEventListenerAdded = false;


this line should be inside removeBackdropEventListeners()

theiliad · 2020-09-14T14:35:48Z

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

-		this.addBackdropEventListeners();
+
+		if (isRulerEnabled && !this.isEventListenerAdded) {
+			this.isEventListenerAdded = true;


this line should be inside addBackdropEventListeners()

…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 linearGradient * 'master' of https://github.com/JennChao/carbon-charts: v0.38.0 Update README.md Update README.md Update README.md Update README.md Update vue.ts Update angular.ts Update react.ts Update vanilla.ts Update README.md Merge pull request carbon-design-system#780 from JennChao/axis-option feat(core): enable or disable ruler (carbon-design-system#765) v0.37.1 Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart

…to tooltip-option * 'master' of https://github.com/JennChao/carbon-charts: v0.38.0 Update README.md Update README.md Update README.md Update README.md Update vue.ts Update angular.ts Update react.ts Update vanilla.ts Update README.md Merge pull request carbon-design-system#780 from JennChao/axis-option feat(core): enable or disable ruler (carbon-design-system#765)

* feat: enable or disable ruler * fix: enable ruler as default setting * refactor: move isRulerEnabled into render * refactor: remove backdrop event listener * refactor: remove code for styling backdrop in ruler component * refactor: flag for checking event listener added status * refactor: refactor ruler component

feat: enable or disable ruler

92397a8

JennChao requested review from natashadecoste, theiliad and a team as code owners August 26, 2020 07:18

JennChao requested review from Donisius and removed request for a team August 26, 2020 07:18

fix: enable ruler as default setting

37a6574

theiliad requested changes Aug 26, 2020

View reviewed changes

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

theiliad requested changes Aug 27, 2020

View reviewed changes

JennChao added 2 commits August 28, 2020 14:50

refactor: move isRulerEnabled into render

c3cc44a

refactor: remove backdrop event listener

8b717ab

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

theiliad requested changes Aug 31, 2020

View reviewed changes

JennChao requested a review from theiliad September 1, 2020 09:26

theiliad requested changes Sep 1, 2020

View reviewed changes

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

80725c4

…to ruler-option

JennChao requested a review from theiliad September 2, 2020 08:38

JennChao added 2 commits September 3, 2020 15:04

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

e9c5602

…to ruler-option

refactor: remove code for styling backdrop in ruler component

5627d39

JennChao mentioned this pull request Sep 7, 2020

feat: enable chart grid enable option #761

Merged

8 tasks

theiliad requested changes Sep 10, 2020

View reviewed changes

JennChao force-pushed the ruler-option branch from 8a82247 to 65a83e0 Compare September 11, 2020 03:23

refactor: flag for checking event listener added status

f1d3f56

JennChao requested a review from theiliad September 11, 2020 03:35

theiliad requested changes Sep 14, 2020

View reviewed changes

JennChao added 2 commits September 15, 2020 15:30

refactor: refactor ruler component

9ee4f1c

JennChao requested a review from theiliad September 15, 2020 07:36

theiliad approved these changes Sep 15, 2020

View reviewed changes

natashadecoste approved these changes Sep 15, 2020

View reviewed changes

sophiiae approved these changes Sep 15, 2020

View reviewed changes

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

theiliad merged commit bef6daf into carbon-design-system:master Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable or disable ruler #765

feat: enable or disable ruler #765

JennChao commented Aug 26, 2020

theiliad left a comment

JennChao commented Aug 27, 2020 •

edited

Loading

theiliad Aug 27, 2020

theiliad Aug 27, 2020

JennChao Aug 28, 2020

theiliad Aug 27, 2020

JennChao Aug 28, 2020 •

edited

Loading

theiliad Aug 31, 2020

JennChao Sep 1, 2020

theiliad Sep 1, 2020

JennChao Sep 2, 2020

JennChao Sep 7, 2020

theiliad Aug 31, 2020

theiliad Sep 1, 2020

theiliad Sep 1, 2020

JennChao Sep 2, 2020

theiliad Sep 10, 2020

JennChao Sep 11, 2020 •

edited

Loading

theiliad Sep 10, 2020

theiliad Sep 14, 2020

theiliad Sep 14, 2020

feat: enable or disable ruler #765

feat: enable or disable ruler #765

Conversation

JennChao commented Aug 26, 2020

Updates

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad left a comment

Choose a reason for hiding this comment

JennChao commented Aug 27, 2020 • 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

JennChao Aug 28, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JennChao Sep 11, 2020 • 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

JennChao commented Aug 27, 2020 •

edited

Loading

JennChao Aug 28, 2020 •

edited

Loading

JennChao Sep 11, 2020 •

edited

Loading