Improved formatting of numeric scale labels #7007

benmccann · 2020-01-23T16:35:41Z

This makes three main changes:

Use commas in large numbers just as proposed in Format numbers in tooltip #7004 for the tooltips
Change the arguments to ticks.callback to be a context object similar to scriptable options
Use the same logic to format numbers on linear and log scales. The log scale previously always used scientific notation, which has never made sense to me. E.g. if you look at the test that changed in scale.logarithmic.tests.js the numbers range from 1 to 80. It's much easier to read these just as normal numbers instead of scientific notation. We still use scientific notation when all the numbers are small.

There's also just some minor changes like using const/let and improving the structure of the if/else checks to bring cognitive complexity down from 9 to 5 to make codeclimate happy

src/core/core.ticks.js

test/specs/scale.radialLinear.tests.js

etimberg · 2020-01-26T20:39:05Z

If we think this will be useful to lots of users, perhaps we can offer it as an option, but not change the defaults? That would give a quick switch but keep the defaults simple (similar idea for #7004)

benmccann · 2020-01-27T21:35:18Z

If we think this will be useful to lots of users, perhaps we can offer it as an option, but not change the defaults? That would give a quick switch but keep the defaults simple

I'd probably rather keep it simpler by having fewer options and just always doing it. I think inserting a comma is a pretty minor change and I'm struggling to think of a usecase where it wouldn't be valuable. If we get feedback during the alpha/beta we can always come back to add an option.

kurkle

Another thing I'm not quite sure about is the placement of the locale configuration. Should it rather be in options? One could want to have charts with different locales on the same page (for example in Chart.js docs).

src/core/core.scale.js

kurkle

There is a mix of Chart.platform.locale and chart.platform.locale (global vs instance).
Is that intended?

benmccann · 2020-01-30T06:09:36Z

No, it wasn't. I updated that. Thanks

kurkle

Still unsure about the placement of locale, but that can be changed later if needed.

benmccann · 2020-01-31T00:31:00Z

Rebased

etimberg

Agree, not hot on placing it on Chart.platform since that was intended to be for switching between browser, basic platform, etc.

I think some documentation should also be written telling users how to use this new option (unless we are intending this only for testing)

etimberg · 2020-01-31T00:29:54Z

src/core/core.ticks.js

-				tickString = '0'; // never show decimal places for 0
+			const maxTick = Math.max(Math.abs(ticks[0].value), Math.abs(ticks[ticks.length - 1].value));
+			const locale = Chart.platform.locale;
+			if (maxTick < 1e-4) { // all ticks are small numbers; use scientific notation


I know the old code didn't do this, but would it be good to do this if all ticks are large as well?

Ok, I've added that

benmccann · 2020-01-31T00:46:23Z

My thinking for putting it on Chart.platform is that is where we've put options that apply at the platform level. I.e. options that affect all charts (which right now is just disableCSSInjection). I'm assuming you wouldn't want charts with different languages on the same page, but maybe that's a bad assumption? What do you think would be a better place to put it? Directly under options?

etimberg · 2020-01-31T00:57:31Z

Personally, I would put under options that way the user can set globally or per chart. I'd rather not limit users since that'll just be a feature request later

benmccann · 2020-01-31T01:22:26Z

Ok, I've moved it to being an option. Unfortunately there's no really good page to document it on, so I've punted on that for now since it at least solves the testing issue

kurkle

While at it, can we change the calling of the callback to be consistent. Either way is fine by me.

src/core/core.scale.js

kurkle · 2020-02-01T05:43:57Z

src/scales/scale.time.js

@@ -652,7 +652,7 @@ class TimeScale extends Scale {
 			tickOpts.callback
 		]);

-		return formatter ? formatter(label, index, ticks) : label;
+		return formatter ? formatter(label, index, ticks, me) : label;


Oh, looks like we are calling the callback inconsistently. Should probably make it consistent.

Yeah, there's multiple things I don't like about this. One of the biggest is that the first parameter is different here. I'd noticed while working on this. It's not that easy to change though and a bit outside the scope of this PR, so probably better handled separately

kurkle · 2020-02-01T05:46:00Z

src/core/core.ticks.js

+			const maxTick = Math.max(Math.abs(ticks[0].value), Math.abs(ticks[ticks.length - 1].value));
+			const minTick = Math.min(Math.abs(ticks[0].value), Math.abs(ticks[ticks.length - 1].value));
+			const locale = scale.chart.options.locale;
+			if (maxTick < 1e-4 || minTick > 1e+7) { // all ticks are small or big numbers; use scientific notation


Don't really like the use of constants here, but this is something that can be altered if it becomes an issue later.

benmccann added the type: enhancement label Jan 23, 2020

benmccann force-pushed the format-ticks branch 2 times, most recently from fcedf3a to 08effe7 Compare January 23, 2020 16:38

etimberg previously approved these changes Jan 25, 2020

View reviewed changes

kurkle requested changes Jan 26, 2020

View reviewed changes

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

test/specs/scale.radialLinear.tests.js Show resolved Hide resolved

benmccann dismissed etimberg’s stale review via 8248916 January 27, 2020 16:44

benmccann force-pushed the format-ticks branch 4 times, most recently from e585af8 to 26b1d00 Compare January 27, 2020 21:28

benmccann force-pushed the format-ticks branch from 26b1d00 to 7e5a3ab Compare January 28, 2020 00:58

benmccann requested review from kurkle and etimberg January 30, 2020 05:11

kurkle reviewed Jan 30, 2020

View reviewed changes

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

benmccann force-pushed the format-ticks branch 2 times, most recently from 8e63e8a to f8b0a69 Compare January 30, 2020 05:55

kurkle reviewed Jan 30, 2020

View reviewed changes

kurkle previously approved these changes Jan 30, 2020

View reviewed changes

Improved formatting of numeric scale labels

0f9f1fa

benmccann dismissed kurkle’s stale review via 0f9f1fa January 31, 2020 00:30

benmccann force-pushed the format-ticks branch from 5f24b97 to 0f9f1fa Compare January 31, 2020 00:30

etimberg reviewed Jan 31, 2020

View reviewed changes

Put locale on options

989bd01

benmccann force-pushed the format-ticks branch from 9875651 to 989bd01 Compare January 31, 2020 01:25

Use scientific notation for big ticks

252c5f9

etimberg previously approved these changes Jan 31, 2020

View reviewed changes

benmccann requested a review from kurkle January 31, 2020 14:28

kurkle reviewed Feb 1, 2020

View reviewed changes

Remove extra parameter

aabf4c3

benmccann dismissed etimberg’s stale review via aabf4c3 February 1, 2020 14:14

kurkle approved these changes Feb 3, 2020

View reviewed changes

etimberg approved these changes Feb 3, 2020

View reviewed changes

etimberg added this to the Version 3.0 milestone Feb 3, 2020

etimberg merged commit f5d9892 into chartjs:master Feb 3, 2020

stockiNail mentioned this pull request Oct 7, 2020

Options.locale documentation is missing #7857

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved formatting of numeric scale labels #7007

Improved formatting of numeric scale labels #7007

benmccann commented Jan 23, 2020 •

edited

Loading

etimberg commented Jan 26, 2020

benmccann commented Jan 27, 2020

kurkle left a comment

kurkle left a comment

benmccann commented Jan 30, 2020

kurkle left a comment

benmccann commented Jan 31, 2020

etimberg left a comment

etimberg Jan 31, 2020

benmccann Jan 31, 2020

benmccann commented Jan 31, 2020

etimberg commented Jan 31, 2020

benmccann commented Jan 31, 2020

kurkle left a comment

kurkle Feb 1, 2020

benmccann Feb 1, 2020

kurkle Feb 1, 2020

Improved formatting of numeric scale labels #7007

Improved formatting of numeric scale labels #7007

Conversation

benmccann commented Jan 23, 2020 • edited Loading

etimberg commented Jan 26, 2020

benmccann commented Jan 27, 2020

kurkle left a comment

Choose a reason for hiding this comment

kurkle left a comment

Choose a reason for hiding this comment

benmccann commented Jan 30, 2020

kurkle left a comment

Choose a reason for hiding this comment

benmccann commented Jan 31, 2020

etimberg left a comment

Choose a reason for hiding this comment

etimberg Jan 31, 2020

Choose a reason for hiding this comment

benmccann Jan 31, 2020

Choose a reason for hiding this comment

benmccann commented Jan 31, 2020

etimberg commented Jan 31, 2020

benmccann commented Jan 31, 2020

kurkle left a comment

Choose a reason for hiding this comment

kurkle Feb 1, 2020

Choose a reason for hiding this comment

benmccann Feb 1, 2020

Choose a reason for hiding this comment

kurkle Feb 1, 2020

Choose a reason for hiding this comment

benmccann commented Jan 23, 2020 •

edited

Loading