feat: provide option to refresh range axis label #766

hlyang397 · 2020-08-26T08:00:06Z

Sometimes data value varies from 10 to 100,000. When zoom bar is moving, the range axis (y-axis) label should be refreshed immediately to avoid extremely small graphs.

Updates

add refreshRangeAxisLabel option (default to false)
add filterDataForRangeAxisLabel() in zoom service to provide data in zoom domain

Demo screenshot or recording

Option disabled (default) :

Option enabled:

Review checklist (for reviewers only)

- add refreshRangeAxisLabel option (default to false) - add filterDataForRangeAxisLabel() in zoom service to provide data in zoom domain

theiliad · 2020-08-26T14:43:48Z

packages/core/src/configuration.ts

@@ -149,7 +149,8 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
 	zoomBar: {
 		top: {
 			enabled: false,
-			type: ZoomBarTypes.GRAPH_VIEW
+			type: ZoomBarTypes.GRAPH_VIEW,
+			refreshRangeAxisLabel: false


Not sure if this makes sense here. What if in top you set refreshRangeAxisLabel to true and in bottom you set it to false? Seems like this would be a zoomBar level config. And it should probably be called updateRangeAxis or realtimeRangeAxis

@theiliad
Thanks for the prompt review.
I think your comment makes sense.
The option name is changed to updateRangeAxis and moved to ZoomBarsOptions in b9abd4c

@theiliad
One more thought, how about moving this configuration toAxisOptions ?

well that depends. It seems like this is only relevant when you have a zoombar, isn't that the case?

Yes, so far this option is required only zoom bar is enabled.
However, putting all axis-related options in the same AxisOptions also makes sense to me.
It's up to you.
Please let me know where you like to set this option, thanks.

packages/core/src/services/scales-cartesian.ts

packages/core/src/services/zoom.ts

theiliad · 2020-09-01T17:27:58Z

@hlyang397 how much have you tested this? what if you start with an initial zoom domain that has a smaller y-axis range and then you zoom-out all the way? Will you see the full range at all times?

hlyang397 · 2020-09-02T02:50:49Z

@hlyang397 how much have you tested this? what if you start with an initial zoom domain that has a smaller y-axis range and then you zoom-out all the way? Will you see the full range at all times?

The y-axis range will be updated whenever zoom domain is changing, so it doesn't matter whether the chart starts from a smaller or bigger range.
Please refer to the demo below.

theiliad · 2020-09-02T16:10:09Z

@hlyang397 how much have you tested this? what if you start with an initial zoom domain that has a smaller y-axis range and then you zoom-out all the way? Will you see the full range at all times?

The y-axis range will be updated whenever zoom domain is changing, so it doesn't matter whether the chart starts from a smaller or bigger range.
Please refer to the demo below.

Yes I'm asking about the case when refreshRangeAxisLabel would be set to false. What would happen then?

hlyang397 · 2020-09-03T03:22:16Z

Yes I'm asking about the case when refreshRangeAxisLabel would be set to false. What would happen then?

@theiliad
Yes, when updateRangeAxis is set to false, you will see the full range at all times, no matter what initial zoom domain is.
updateRangeAxis is default to false, so all storybook demos should look same as before.

theiliad · 2020-09-10T18:47:50Z

packages/core/src/interfaces/components.ts

+	/**
+	 * whether keep updating range axis in real time while zoom domain is changing
+	 */
+	updateRangeAxis?: boolean;


I've already commented on this before, not sure where my comment went (usually they disappear if you force-push over the branch and the lines that were commented on get removed).

This seems like it should just be flag under:

{ axes: { "top|left|bottom|right": { update: boolean; } } }

whether it needs to be called update or updateWhenZooming depends on whether this only applies to zooming or not

I will change it to updateWhenZooming since the axis won't change when zoom bar is disabled.
Updated in a224ab3.

hlyang397 · 2020-09-11T06:45:30Z

@theiliad I don't think we have any force-push in this PR. In that case, we should be able to find the force-push record like this:

theiliad · 2020-09-11T15:27:19Z

@theiliad I don't think we have any force-push in this PR. In that case, we should be able to find the force-push record like this:

yes, what I was saying about this is that if I comment on line X and that line has been removed in a force-pushed commit, then the comment seems to get deleted forever on Github.

hlyang397 · 2020-09-12T07:15:25Z

@theiliad So any other comment on this PR?

theiliad

A bit unhappy that this is really just focusing on the main y-axis and not also the main x-axis, however I can't stand in the way of your progress

sophiiae · 2020-09-15T16:20:32Z

@hlyang397 please fix the conflicts

# Conflicts: # packages/core/src/interfaces/axis-scales.ts

hlyang397 · 2020-09-16T04:15:43Z

A bit unhappy that this is really just focusing on the main y-axis and not also the main x-axis, however I can't stand in the way of your progress

@theiliad We don't have vertical zoom bar yet, so I can't test this feature on main x-axis, and that's why I focus on main y-axis for now.

hlyang397 · 2020-09-16T04:18:17Z

@hlyang397 please fix the conflicts

@sophiiae Latest master branch merged. Thanks.

# Conflicts: # packages/core/src/services/zoom.ts

cal-smith

LGTM!

theiliad · 2020-10-05T14:15:23Z

packages/core/src/services/zoom.ts

+			this.model.getOptions(),
+			"axes",
+			this.services.cartesianScales.getMainYAxisPosition(),
+			"updateWhenZooming"


I was just testing this and realized that updateWhenZooming is available for all axes, however it only takes effect for the main Y-axis. this means that I can set it to false for the main X-axis and it will do nothing

we should either just make it available for the range axis like before, or if we are allowing it to be used for all axis then it should achieve something when set to false

@theiliad The domain axis is always changing while zoom domain is changing, so no option is required.
This new option should only works for the range axis (main Y axis).
So I change it back to updateRangeAxis under zoomBar.

Updated in 2cd37ad.

Please let me know if this is not what you expected.

hlyang397 · 2020-10-07T05:08:28Z

@theiliad Not sure what happened to Travis CI, I can pass the test locally.
Could you help to rerun the build in Travis CI ? Thanks.

…m#766) * feat: provide option to refresh range axis label - add refreshRangeAxisLabel option (default to false) - add filterDataForRangeAxisLabel() in zoom service to provide data in zoom domain * refactor: move updateRangeAxis to zoomBarsOptions * refactor: use configs object instead of boolean as parameter * refactor: move updateWhenZooming option to be under axis option * refactor: change updateWhenZooming option to zoomBar.updateRangeAxis

feat: provide option to refresh range axis label

4f00d2f

- add refreshRangeAxisLabel option (default to false) - add filterDataForRangeAxisLabel() in zoom service to provide data in zoom domain

hlyang397 requested review from natashadecoste, theiliad and a team as code owners August 26, 2020 08:00

hlyang397 requested review from cal-smith and removed request for a team August 26, 2020 08:00

theiliad requested changes Aug 26, 2020

View reviewed changes

refactor: move updateRangeAxis to zoomBarsOptions

b9abd4c

theiliad requested changes Aug 27, 2020

View reviewed changes

packages/core/src/services/scales-cartesian.ts Outdated Show resolved Hide resolved

packages/core/src/services/zoom.ts Outdated Show resolved Hide resolved

refactor: use configs object instead of boolean as parameter

9688034

hlyang397 requested a review from theiliad August 28, 2020 03:38

natashadecoste previously approved these changes Sep 1, 2020

View reviewed changes

hlyang397 mentioned this pull request Sep 8, 2020

Tool bar above zoom bar and shift-clicking on chart #717

Closed

8 tasks

theiliad requested changes Sep 10, 2020

View reviewed changes

hlyang397 added 2 commits September 11, 2020 14:55

Merge branch 'master' into scalable-y-value

056614f

refactor: move updateWhenZooming option to be under axis option

a224ab3

hlyang397 dismissed natashadecoste’s stale review via a224ab3 September 11, 2020 07:45

hlyang397 requested review from theiliad and natashadecoste September 11, 2020 07:51

theiliad previously approved these changes Sep 15, 2020

View reviewed changes

natashadecoste previously approved these changes Sep 15, 2020

View reviewed changes

Merge branch 'master' into scalable-y-value

180b0b5

# Conflicts: # packages/core/src/interfaces/axis-scales.ts

hlyang397 dismissed stale reviews from natashadecoste and theiliad via 180b0b5 September 16, 2020 04:10

hlyang397 requested review from natashadecoste and theiliad September 16, 2020 04:16

theiliad previously approved these changes Sep 16, 2020

View reviewed changes

natashadecoste previously approved these changes Sep 16, 2020

View reviewed changes

Merge branch 'master' into scalable-y-value

387c68b

# Conflicts: # packages/core/src/services/zoom.ts

hlyang397 dismissed stale reviews from natashadecoste and theiliad via 387c68b September 21, 2020 05:50

hlyang397 requested review from natashadecoste and theiliad September 21, 2020 05:53

theiliad approved these changes Sep 21, 2020

View reviewed changes

cal-smith previously approved these changes Sep 30, 2020

View reviewed changes

theiliad requested changes Oct 5, 2020

View reviewed changes

refactor: change updateWhenZooming option to zoomBar.updateRangeAxis

2cd37ad

hlyang397 dismissed cal-smith’s stale review via 61ad8a8 October 6, 2020 03:07

hlyang397 requested review from theiliad and cal-smith October 6, 2020 03:15

Merge branch 'master' into scalable-y-value

858b62f

hlyang397 force-pushed the scalable-y-value branch from 61ad8a8 to 858b62f Compare October 6, 2020 03:18

theiliad approved these changes Oct 6, 2020

View reviewed changes

sophiiae approved these changes Oct 6, 2020

View reviewed changes

theiliad merged commit 070f9e3 into carbon-design-system:master Oct 8, 2020

theiliad deleted the scalable-y-value branch October 8, 2020 16:11

theiliad added the type: enhancement 💡 label Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: provide option to refresh range axis label #766

feat: provide option to refresh range axis label #766

hlyang397 commented Aug 26, 2020

theiliad Aug 26, 2020

hlyang397 Aug 27, 2020 •

edited

Loading

hlyang397 Aug 27, 2020

theiliad Aug 27, 2020

hlyang397 Aug 28, 2020 •

edited

Loading

theiliad commented Sep 1, 2020

hlyang397 commented Sep 2, 2020

theiliad commented Sep 2, 2020

hlyang397 commented Sep 3, 2020

theiliad Sep 10, 2020

hlyang397 Sep 11, 2020 •

edited

Loading

hlyang397 commented Sep 11, 2020

theiliad commented Sep 11, 2020

hlyang397 commented Sep 12, 2020

theiliad left a comment

sophiiae commented Sep 15, 2020

hlyang397 commented Sep 16, 2020

hlyang397 commented Sep 16, 2020

cal-smith left a comment

theiliad Oct 5, 2020

theiliad Oct 5, 2020

hlyang397 Oct 6, 2020

hlyang397 commented Oct 7, 2020

feat: provide option to refresh range axis label #766

feat: provide option to refresh range axis label #766

Conversation

hlyang397 commented Aug 26, 2020

Updates

Demo screenshot or recording

Review checklist (for reviewers only)

theiliad Aug 26, 2020

Choose a reason for hiding this comment

hlyang397 Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

hlyang397 Aug 27, 2020

Choose a reason for hiding this comment

theiliad Aug 27, 2020

Choose a reason for hiding this comment

hlyang397 Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

theiliad commented Sep 1, 2020

hlyang397 commented Sep 2, 2020

theiliad commented Sep 2, 2020

hlyang397 commented Sep 3, 2020

theiliad Sep 10, 2020

Choose a reason for hiding this comment

hlyang397 Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

hlyang397 commented Sep 11, 2020

theiliad commented Sep 11, 2020

hlyang397 commented Sep 12, 2020

theiliad left a comment

Choose a reason for hiding this comment

sophiiae commented Sep 15, 2020

hlyang397 commented Sep 16, 2020

hlyang397 commented Sep 16, 2020

cal-smith left a comment

Choose a reason for hiding this comment

theiliad Oct 5, 2020

Choose a reason for hiding this comment

theiliad Oct 5, 2020

Choose a reason for hiding this comment

hlyang397 Oct 6, 2020

Choose a reason for hiding this comment

hlyang397 commented Oct 7, 2020

hlyang397 Aug 27, 2020 •

edited

Loading

hlyang397 Aug 28, 2020 •

edited

Loading

hlyang397 Sep 11, 2020 •

edited

Loading