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

BUG - src/platforms/platform.dom.js does not support decimals in px #3860

Closed
qsbpetf opened this issue Feb 2, 2017 · 5 comments
Closed

BUG - src/platforms/platform.dom.js does not support decimals in px #3860

qsbpetf opened this issue Feb 2, 2017 · 5 comments
Assignees
Milestone

Comments

@qsbpetf
Copy link

qsbpetf commented Feb 2, 2017

Width or height in pixels can be decimal, e.g. 1163.34px and must be supported which this lib does not support.

Expected Behavior

1163.34px should return 1163

Current Behavior

It returns 34, e.g. only the fractional part. This is due to an error in the regexp.

Possible Solution

Line 36 in src/platforms/platform.dom.js needs to be changed from:
var matches = value && value.match(/(\d+)px/);
into:
var matches = value && value.match(/(\d+)(.\d+)?px/);

Steps to Reproduce (for bugs)

Just use a browser Chrome and do some resizing of the screen.

Context

The diagram sometimes get 10000 px height when 1000px width while it should be 300px height. This is due du aspect ratio in combination with reading only fractional part of px value.

Environment

  • Chart.js version: 2.4.0
  • Browser name and version: Chrome 55.0.2883.95 (64-bit)
@simonbrunel
Copy link
Member

Nice catch @qsbpetf. I think we can make the regexp even more accurate: /^(\d+)(?:\.\d+)?px$/ (we explicitly want a . (dot) as separator (so need to be escaped) and it should start by a number ^ and ends by px$/. I don't know what the browser returns in case of a floating number between 0 and 1: '0.42px' or '.42px' (this regexp only matches the first version).

@qsbpetf
Copy link
Author

qsbpetf commented Feb 2, 2017

I fully agree.
When I patched the lib locally I had the escape \ before the dot :) Must have missed it during the writing of this bug.
Thanks for an awesome lib!

@etimberg
Copy link
Member

etimberg commented Feb 3, 2017

@qsbpetf is it possible to PR a fix for this?

@qsbpetf
Copy link
Author

qsbpetf commented Feb 3, 2017

@etimberg I didn't fork the code from GH nor built it. I just DLed the full .js file, single step debugged it in the browser, patched the. js file and then ran it successfully :)

@simonbrunel
Copy link
Member

@etimberg I will submit a PR, thanks @qsbpetf for the report :)

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