Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time Scale should align to beginning of time period #4187

Closed
benmccann opened this issue Apr 27, 2017 · 7 comments
Closed

Time Scale should align to beginning of time period #4187

benmccann opened this issue Apr 27, 2017 · 7 comments

Comments

@benmccann
Copy link
Contributor

benmccann commented Apr 27, 2017

This can be reproduced with the current samples in master

Current Behavior

Samples show:

  • Apr 26 9PM, Apr 27 9AM, Apr 27 9PM, etc.
  • Apr 26 9PM, Apr 27 3AM, Apr 27 9AM, etc.

Expected Behavior

It would be nicer if these instead said:

  • Apr 26, 12PM, Apr 27, 12PM, Apr 28, etc.
  • Apr 26, 6AM, 12PM, 6PM, Apr 27, etc.

It's much easier to read the chart when it aligns on full days (or hours, weeks, months, etc.). E.g. in this case we should always try to have some labels at midnight as 9am and 9pm are weird times to align our labels to.

Also, much of the information is repetitive. You only need to print the major time period when you hit it and only need to print the minor time period in between. We should at least have an option that allows for this if we don't always do it (and I think setting it to true by default would be nice)

Steps to Reproduce (for bugs)

  1. git clone git@github.com:chartjs/Chart.js.git
  2. npm install
  3. gulp build
  4. Open samples/scales/time/combo.html and samples/scales/time/line-point-data.html

Examples

https://www.amcharts.com/stock-chart/

Here are a couple screenshots from other time series charts
example1
example2

@etimberg
Copy link
Member

I agree that that is probably a useful feature. I don't know if by making this change existing users would be unhappy. Do you think something like this should live behind a configuration setting?

@benmccann
Copy link
Contributor Author

For half of it. There are two things I asked for here:

  1. Alignment on major time periods - I think this should always happen. I don't know how to describe the current behavior which seems to be random. I think aligned on time period always makes more sense than aligned randomly
  2. Printing of labels for minor and major ticks differently. This one I was thinking probably makes sense to be an option

@etimberg
Copy link
Member

I can agree to part 1 being always on. I think it should actually be relatively straightforward because all I think needs to be done is taking away a few ifs.

https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js#L251
https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js#L306

and then making sure we round to the correct unit rather than the value of the round setting. The setting is used to round data points as well.

Part 2 is more complicated. Right now the axis only has the concept of tick marks. We'd need to introduce the concept of minor tick marks. I don't think it's actually too bad other than it requires duplicating a bunch of settings so they can be configured independently and having a second array of ticks (or storing the type of tick in the items in scale.ticks. I think the meat of the changes might be confined to just this loop https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L550-L651

@benmccann
Copy link
Contributor Author

benmccann commented Apr 28, 2017

I don't think we actually need to expose any concept of major/minor tick marks to the user in which case it wouldn't necessarily be that much more complicated. We already have the unit from determineUnit. So we just need to see if the time is at the beginning of that unit (or maybe the next larger or smaller unit instead. I haven't looked quite closely enough) and format a bit differently depending on whether it's the beginning of the unit or not.

@benmccann benmccann reopened this Apr 28, 2017
@etimberg
Copy link
Member

That's actually a good idea. Should be relatively straightforward to do and changing the text might actually already be possible with the label callback. Changing the styling definitely requires some code changes

@SesioN
Copy link

SesioN commented May 12, 2017

+1 on this idea! I like what I read.

@benmccann
Copy link
Contributor Author

I've documented how this should be implemented from a technical perspective in #4612. I'm going to close this issue in favor of that one, which has a lot more technical detail and should be much more helpful to an implementor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants