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

Allow specifying labels in time scale options #6257

Merged
merged 5 commits into from May 21, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 8, 2019

Currently time scale only reads labels from data.labels.
If both axes are time scales, this is not sufficient.

So this PR allows specifying labels under scale options also.

@benmccann
Copy link
Contributor

@kurkle this only updates the docs, but from the PR description I would expect code to have changed as well. Did you accidentally leave some changes off of the PR?

@kurkle
Copy link
Member Author

kurkle commented May 8, 2019

@kurkle this only updates the docs, but from the PR description I would expect code to have changed as well. Did you accidentally leave some changes off of the PR?

Its so small change, you missed it. Had any ☕️ yet 😄

@benmccann
Copy link
Contributor

Haha. Sorry

This change looks fine to me, but I'm curious why ticks.source: 'data' wouldn't allow you to accomplish the same thing?

The other thing I wonder is if any other scales would need this same functionality

@etimberg
Copy link
Member

etimberg commented May 8, 2019

I wonder if we can extend the same pattern to the category scale and close out #3193 too

etimberg
etimberg previously approved these changes May 8, 2019
@kurkle
Copy link
Member Author

kurkle commented May 9, 2019

I wonder if we can extend the same pattern to the category scale and close out #3193 too

category scale already supports this, and additionally xLabes/yLabels:

getLabels: function() {
var data = this.chart.data;
return this.options.labels || (this.isHorizontal() ? data.xLabels : data.yLabels) || data.labels;
},

Not quite sure how this would solve #3193?

@kurkle
Copy link
Member Author

kurkle commented May 9, 2019

I'm curious why ticks.source: 'data' wouldn't allow you to accomplish the same thing?

Can't find the chart right now , but for example if there is no data for all desired labels.

benmccann
benmccann previously approved these changes May 10, 2019
src/scales/scale.time.js Outdated Show resolved Hide resolved
@kurkle kurkle dismissed stale reviews from benmccann and etimberg via 8d21f50 May 11, 2019 12:49
etimberg
etimberg previously approved these changes May 11, 2019
src/core/core.scale.js Outdated Show resolved Hide resolved
benmccann
benmccann previously approved these changes May 11, 2019
@simonbrunel simonbrunel added this to the Version 2.9 milestone May 12, 2019
@nagix
Copy link
Contributor

nagix commented May 13, 2019

@kurkle You might want to update the time scale document?

etimberg
etimberg previously approved these changes May 13, 2019
simonbrunel
simonbrunel previously approved these changes May 15, 2019
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'm curious why linear scales don't need any change to support labels in the scale options?

@kurkle
Copy link
Member Author

kurkle commented May 15, 2019

Looks good! I'm curious why linear scales don't need any change to support labels in the scale options?

linear scales just don't support labels in any way (yet). I'd leave that for another PR.

@simonbrunel
Copy link
Member

I thought linear scales was using labels to determine the position the x position of each data:

data: {
    labels: [0, 2, 6, 8],
    datasets: [{
        data: [1, 4, 8, 2]
    }]
}

// e.q.
data: [
    {x: 0, y: 1},
    {x: 2, y: 4},
    {x: 6, y: 8},
    {x: 8, y: 2}
]

@kurkle
Copy link
Member Author

kurkle commented May 15, 2019

@simonbrunel It does not work: Pen
'line' chart type defaults to category as x-axis, could that be why you thought linear scales support labels?.

xAxes: [{
type: 'category',
id: 'x-axis-0'

@simonbrunel
Copy link
Member

@kurkle can we also add unit tests?

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants