Allow axes to be centered on the chart area #6818

etimberg · 2019-12-08T15:20:50Z

Based on #2960
Resolves #2164

Center horizontal scales draw in the center of the chart area, rather than at the bottom.

To Do

Center horizontal scales draw in the center of the chart area, rather than at the bottom

kurkle

Looks like this is going to be fairly easy

src/core/core.layouts.js

etimberg · 2019-12-08T20:45:20Z

I added support for center Y axes too

kurkle

Some nits

src/core/core.controller.js

src/core/core.scale.js

src/core/core.controller.js

etimberg · 2019-12-08T23:18:58Z

@kurkle I have the position='y=..' mode working locally. You have to specify a positionAxisID so that the axis knows how to look up the position using the other axis. Results for position='y=-20' are below

Edit: Now have x= mode working for vertical axes

Vertical axes can be moved left and right by specifying a position of `x=...` Horizontal axes can be moved up and down by specifying a position of `y=...` Update sample to show multiple possible positions

etimberg · 2019-12-09T00:40:22Z

I pushed up support for y=... and x=... modes. Includes documentation & samples. Not sure what kinds of tests are appropriate here. All the text would have to be turned off for an image test

kurkle · 2019-12-09T05:09:32Z

The variable position: Maybe use object instead,
position: {y: 500}
It would allow position: {x: 10, y: 50} for radial scale.
We could require the key to be axisID, removing the need for extra option for that.
(I did not actually look at the code yet, just pondering the config format)

etimberg · 2019-12-09T12:07:45Z

I'll have to look and see how that goes. It might work, but I can foresee some validation challenges. I won't get to this until tomorrow (Dec. 10) evening so I can ponder on it until then

kurkle

quick read

kurkle · 2019-12-09T12:41:31Z

src/core/core.scale.js

 			borderValue = alignBorderValue(me.left);
 			x1 = chartArea.left;
 			x2 = alignBorderValue(chartArea.right) - axisHalfWidth;
 			tx1 = borderValue + axisHalfWidth;
 			tx2 = me.left + tl;
+		} else if (axis === 'x') {


I think I'd rather put this logic in layouts and set a variable, borderPixel for example, to the scale (box). And use the variable here and in _computeLabelItems.

or just set the top/bottom correctly and use those here as if this was a bottom scale.

My initial thought was to use the layout system as well. The reason I didn't was that it would mean that the layout system would have to differentiate scales from other boxes. I wasn't sure if that was going to work long-term or not

src/core/core.controller.js

kurkle · 2019-12-09T13:00:00Z

Another question, does the a centered scale still reduce chartArea?

This avoids needing to parse a config string.

etimberg · 2019-12-13T02:39:41Z

I updated this to use this format for the complex positions

position: {
  y: 20
}

kurkle · 2019-12-13T07:56:08Z

Some lint errors, but looks good!

etimberg · 2019-12-13T12:17:35Z

Sweet, I fixed the lint warnings

kurkle

Another quick look and not much to comment. Needs some tests, maybe couple image based without labels?

Edit: Oh, and some tests are failing

docs/axes/cartesian/README.md

etimberg · 2019-12-14T04:53:52Z

Will look into the test failures. Not sure what I broke.

etimberg · 2019-12-14T18:22:44Z

I fixed the test failures. The cause was defaulting the axis option based on id[0]. Because if its x and the axis has a default position of left the code doesn't reassign the scale to the bottom.

For now, I removed the id[0] default. We could try removing the axis position defaults from the scale defaults and rely on the default position setting

etimberg · 2019-12-16T15:52:43Z

I've added fixture based tests for this so its good to review

benmccann · 2019-12-16T16:49:48Z

Looks good to me. I still think it'd be nice if axis weren't required, but otherwise am happy with it

etimberg · 2019-12-16T18:49:05Z

@benmccann I removed the axis defaults, and handled it correctly. Now we can use the first character of the axis ID to determine if its horizontal or vertical

docs/axes/cartesian/README.md

src/core/core.controller.js

benmccann · 2019-12-16T20:08:55Z

src/core/core.controller.js

-function positionIsHorizontal(position) {
-	return position === 'top' || position === 'bottom';
+const WELL_KNOWN_POSITIONS = ['top', 'bottom', 'left', 'right', 'chartArea'];
+function positionIsHorizontal(position, axis) {


can we just do axis === 'x' instead?

We might now that I removed default positions. Before I did that, it would override the fact that the position was wrong and the config would not be updated.

Tried this. It breaks the default position fixing because it overrides the wrong axis position value. Lets leave as is for now

Allow center horizontal scales.

ea5804b

Center horizontal scales draw in the center of the chart area, rather than at the bottom

kurkle reviewed Dec 8, 2019

View reviewed changes

src/core/core.layouts.js Outdated Show resolved Hide resolved

src/core/core.layouts.js Outdated Show resolved Hide resolved

etimberg added 3 commits December 8, 2019 11:01

Fix incorrect variable

524329c

Update to use position of 'center' with axis option of 'x'

02deb53

Support center vertical axes

f643a7e

Add sample file

74ce364

kurkle reviewed Dec 8, 2019

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

src/core/core.scale.js Outdated Show resolved Hide resolved

etimberg added 4 commits December 8, 2019 16:22

Add some documentation

db4ac3c

Sample linting

7abb001

Address code review feedback

e63a6a0

Remove debug border

efe48a9

etimberg marked this pull request as ready for review December 8, 2019 21:33

benmccann reviewed Dec 8, 2019

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

Support dynamic axis positions based on a value

5943d2a

Vertical axes can be moved left and right by specifying a position of `x=...` Horizontal axes can be moved up and down by specifying a position of `y=...` Update sample to show multiple possible positions

kurkle reviewed Dec 9, 2019

View reviewed changes

etimberg added 3 commits December 12, 2019 21:35

Use objects for complex positions.

fcc6b7b

This avoids needing to parse a config string.

Remove unnecesary default

5d597a8

Documentation updates

ab9cc7b

Fix lint warnings

15e7016

kurkle reviewed Dec 13, 2019

View reviewed changes

docs/axes/cartesian/README.md Outdated Show resolved Hide resolved

Do not default position based on ID

919571a

Add fixture tests for axis positioning

cebe819

Remove default scale positions

47efdd2

Update docs per code review

259b926

benmccann reviewed Dec 16, 2019

View reviewed changes

docs/axes/cartesian/README.md Outdated Show resolved Hide resolved

benmccann reviewed Dec 16, 2019

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

Code review feedback

2d07389

benmccann reviewed Dec 16, 2019

View reviewed changes

kurkle approved these changes Dec 16, 2019

View reviewed changes

benmccann approved these changes Dec 16, 2019

View reviewed changes

benmccann added the type: enhancement label Dec 16, 2019

etimberg merged commit 374b749 into chartjs:master Dec 16, 2019

etimberg deleted the scales-middle-pos branch December 16, 2019 23:17

etimberg added this to the Version 3.0 milestone Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow axes to be centered on the chart area #6818

Allow axes to be centered on the chart area #6818

etimberg commented Dec 8, 2019 •

edited

Loading

kurkle left a comment

etimberg commented Dec 8, 2019

kurkle left a comment

etimberg commented Dec 8, 2019 •

edited

Loading

etimberg commented Dec 9, 2019

kurkle commented Dec 9, 2019

etimberg commented Dec 9, 2019

kurkle left a comment

kurkle Dec 9, 2019

kurkle Dec 9, 2019

etimberg Dec 13, 2019

kurkle commented Dec 9, 2019

etimberg commented Dec 13, 2019

kurkle commented Dec 13, 2019

etimberg commented Dec 13, 2019

kurkle left a comment •

edited

Loading

etimberg commented Dec 14, 2019

etimberg commented Dec 14, 2019

etimberg commented Dec 16, 2019

benmccann commented Dec 16, 2019

etimberg commented Dec 16, 2019

benmccann Dec 16, 2019

etimberg Dec 16, 2019

etimberg Dec 16, 2019

Allow axes to be centered on the chart area #6818

Allow axes to be centered on the chart area #6818

Conversation

etimberg commented Dec 8, 2019 • edited Loading

To Do

kurkle left a comment

Choose a reason for hiding this comment

etimberg commented Dec 8, 2019

kurkle left a comment

Choose a reason for hiding this comment

etimberg commented Dec 8, 2019 • edited Loading

etimberg commented Dec 9, 2019

kurkle commented Dec 9, 2019

etimberg commented Dec 9, 2019

kurkle left a comment

Choose a reason for hiding this comment

kurkle Dec 9, 2019

Choose a reason for hiding this comment

kurkle Dec 9, 2019

Choose a reason for hiding this comment

etimberg Dec 13, 2019

Choose a reason for hiding this comment

kurkle commented Dec 9, 2019

etimberg commented Dec 13, 2019

kurkle commented Dec 13, 2019

etimberg commented Dec 13, 2019

kurkle left a comment • edited Loading

Choose a reason for hiding this comment

etimberg commented Dec 14, 2019

etimberg commented Dec 14, 2019

etimberg commented Dec 16, 2019

benmccann commented Dec 16, 2019

etimberg commented Dec 16, 2019

benmccann Dec 16, 2019

Choose a reason for hiding this comment

etimberg Dec 16, 2019

Choose a reason for hiding this comment

etimberg Dec 16, 2019

Choose a reason for hiding this comment

etimberg commented Dec 8, 2019 •

edited

Loading

etimberg commented Dec 8, 2019 •

edited

Loading

kurkle left a comment •

edited

Loading