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

Vertical time scale generates too few ticks #6258

Merged
merged 3 commits into from May 12, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 8, 2019

Vertical time scale generates too few ticks, due to considering tick length in capacity calculation.

This might not be a correct fix however, because the function is called getLabelWidth.

Master: Pen
image

PR: Pen
image

With offset: Pen

benmccann
benmccann previously approved these changes May 8, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

getLabelWidth is marked @private so I would be okay renaming it to _getLabelSize

@simonbrunel
Copy link
Member

It's marked private but not since the beginning (introduced in v2.6 and made private in v2.7) so not sure it's a good idea to rename it since it doesn't add any benefit.

@simonbrunel simonbrunel added this to the Version 2.9 milestone May 8, 2019
simonbrunel
simonbrunel previously approved these changes May 8, 2019
@simonbrunel
Copy link
Member

Though, good point from @kurkle that the returned value isn't anymore the label width for vertical scales. Introducing _getLabelSize (private) returning {w, h} and have getLabelWidth deprecated and be return _getLabelSize().w instead seems a good alternative?

@benmccann
Copy link
Contributor

I want to know how @kurkle got time travel abilities :-)

Screenshot from 2019-05-08 09-31-21

@kurkle kurkle dismissed stale reviews from simonbrunel and benmccann via 1f63c38 May 8, 2019 16:59
benmccann
benmccann previously approved these changes May 8, 2019
@kurkle
Copy link
Member Author

kurkle commented May 8, 2019

Another thing noticed here is that offset is not accounted for in chartArea. Box size is calculated from chartArea in that example, that's why it looks so wrong in master.

@kurkle
Copy link
Member Author

kurkle commented May 8, 2019

For 2.8.0 this can be worked around by setting maxRotation: 90 on vertical time scale.

@nagix
Copy link
Contributor

nagix commented May 9, 2019

If we take into account offset, the code would look like this?

var capacity = Math.floor(me.isHorizontal() ? me.width / size.w : me.height / size.h);

if (me.options.offset) {
    capacity--;
}

return capacity > 0 ? capacity : 1;

@kurkle
Copy link
Member Author

kurkle commented May 9, 2019

@nagix I think you are right. I'll test that when I have a chance.

@kurkle
Copy link
Member Author

kurkle commented May 9, 2019

Made @nagix suggestion and then realized its not strictly correct either. If min/max is defined, offset is ignored at that end. Doesn't make a huge difference though.

@nagix
Copy link
Contributor

nagix commented May 9, 2019

Ok, this is related to #5618. If everyone agrees that offset should work even when min or max is set, it should be fixed separately.

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

4 participants