- 
                Notifications
    
You must be signed in to change notification settings  - Fork 197
 
formatNumber; filter log tickFormat function #2078
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
Conversation
507264a    to
    d9a98e9      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the feature — I hope we'll find a way to solve #384 too!
Two small comments/questions.
| // to infer whether the ordinal scale is UTC or local time. | ||
| export function inferTickFormat(scale, data, ticks, tickFormat, anchor) { | ||
| return typeof tickFormat === "function" | ||
| return typeof tickFormat === "function" && !(scale.type === "log" && scale.tickFormat) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return typeof tickFormat === "function" && !(scale.type === "log" && scale.tickFormat) | |
| return typeof tickFormat === "function" && scale.type !== "log" | 
Nit: I'm not sure about the case for the second part of the test. When scale.type is "log", the scale is an instance of d3.scaleLog() and it has a tickFormat method, which the user can't nullify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t do this because I didn’t want to assume that log scales necessarily implement scale.tickFormat. That is currently true, but if this assumption doesn’t hold in the future, this would break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error may be mine, but this "invalid" message comes from the format in many European languages returning 2 000 which is not accepted in SVG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem comes from running beautify.html on the generated SVG, meaning that the SVG is assumed to be in HTML. We’ll have to find a way to disable that for tests.
Edit: Actually it’s from JSDOM’s outerHTML serialization.
Fixes #2077. Also exposes Plot.formatNumber(locale).