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

Feature separatethousands #848

Merged
merged 12 commits into from Aug 15, 2016
Merged

Feature separatethousands #848

merged 12 commits into from Aug 15, 2016

Conversation

satotake
Copy link
Contributor

@satotake satotake commented Aug 12, 2016

related #841
(codepen example)[http://codepen.io/satotake/pen/wWRpbd]
It seems inconsistent that 4-digit numbers on axes such as 3000 has no comma separation while more than 5-digit numbers are separated with commas.

This PR fix this issue.
If explicit flag separatethousands givien to layouts, 4-digit integers are applied to comma separation.

Please review it.
Maybe additional tests are required...

@etpinard
Copy link
Contributor

etpinard commented Aug 12, 2016

Great PR. Thanks!

At first, I thought having separators in layout while separatethousands in axis looked a little off. But, I guess, separatethousands is closer to exponentformat, ticksuffixes etc than the global separators. So 👍 there.

Some tests are failing due to the rather strict Lib.coerce design. I'll proposed a solution below the line in question.

It would be nice to add an image test for separatethousands. To do so, add an image test mock JSON in the test/image/mocks/ directory showing off this new functionality. If you're feeling adventurous, you can try to generate the baseline yourself in our image test docker container (docs here).

@@ -43,6 +43,7 @@ module.exports = function handleTickLabelDefaults(containerIn, containerOut, coe
if(!tickFormat && axType !== 'date') {
coerce('showexponent', showAttrDflt);
coerce('exponentformat');
coerce('separatethousands');
Copy link
Contributor

Choose a reason for hiding this comment

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

Lib.coerce fails if a passed prop (here separatethousands) isn't part of the attribute object used to defined the coerce wrapper function above.

So, we need to make sure that separatethousands is part of all attribute objects that pass through here. I believe this mean adding seperatethousands in:

Moreover, to enable seperatethousands in color bars, you'll need to pass to the mock axis here.

@satotake
Copy link
Contributor Author

satotake commented Aug 13, 2016

todo

  • fix description in layout_attributes
  • add mock json
  • generate the baseline with docker
  • add separatethousands to the other types of graph

@satotake
Copy link
Contributor Author

All done!

  • plotly.js works in html as expected
  • test/images/mocks and baseline image added
  • CI test passed
    Is this okay for you, @etpinard ?

@etpinard
Copy link
Contributor

@satotake amazing PR. Thanks very much for your efforts. 🍻

@etpinard etpinard merged commit ad88139 into plotly:master Aug 15, 2016
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

2 participants