You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by minahlee <gi...@git.apache.org> on 2016/02/02 23:19:40 UTC
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
GitHub user minahlee opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/685
[ZEPPELIN-595] Allow displaying decimal format in d3
### What is this PR for?
This PR enables displaying decimal format in built-in nvd3 chart
### What type of PR is it?
Bug Fix
### Is there a relevant Jira issue?
[ZEPPELIN-595](https://issues.apache.org/jira/browse/ZEPPELIN-595)
### Screenshots (if appropriate)
Before
<img width="767" alt="screen shot 2016-02-02 at 2 12 38 pm" src="https://cloud.githubusercontent.com/assets/8503346/12766225/836f42b2-c9b7-11e5-8dd2-9135d76324b0.png">
After
<img width="767" alt="screen shot 2016-02-02 at 2 13 17 pm" src="https://cloud.githubusercontent.com/assets/8503346/12766226/848996e8-c9b7-11e5-8104-b6654686dcd4.png">
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/minahlee/incubator-zeppelin ZEPPELIN-595
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/685.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #685
----
commit 834fc1364fbbb57832d38a605efe7779921250d0
Author: Mina Lee <mi...@nflabs.com>
Date: 2016-02-02T22:10:37Z
Allow displaying decimal d3 format
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179431380
@corneadoug Thank you for review. Just pushed a fix
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-181183622
LGTM. thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179666377
My bad, searched into a wrong directory; it was under reports. Please ignore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179438556
Since its the only problem I found, I would say LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-180837936
My bad. I just pushed a fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179030608
For reviewer's information, please check [this link](https://travis-ci.org/apache/incubator-zeppelin/builds/106608051) to see CI passed. I triggered CI twice, first one failed, second one suceeded. But the first CI test finished later than second one that's why build indicated as failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-180270571
Awesome!!!
LGTM :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
GitHub user minahlee reopened a pull request:
https://github.com/apache/incubator-zeppelin/pull/685
[ZEPPELIN-595] Allow displaying decimal format in d3
### What is this PR for?
This PR enables displaying decimal format in built-in nvd3 chart
### What type of PR is it?
Bug Fix
### Is there a relevant Jira issue?
[ZEPPELIN-595](https://issues.apache.org/jira/browse/ZEPPELIN-595)
### Screenshots (if appropriate)
Before
<img width="767" alt="screen shot 2016-02-02 at 2 12 38 pm" src="https://cloud.githubusercontent.com/assets/8503346/12766225/836f42b2-c9b7-11e5-8dd2-9135d76324b0.png">
After
<img width="767" alt="screen shot 2016-02-02 at 2 13 17 pm" src="https://cloud.githubusercontent.com/assets/8503346/12766226/848996e8-c9b7-11e5-8104-b6654686dcd4.png">
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/minahlee/incubator-zeppelin ZEPPELIN-595
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/685.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #685
----
commit 834fc1364fbbb57832d38a605efe7779921250d0
Author: Mina Lee <mi...@nflabs.com>
Date: 2016-02-02T22:10:37Z
Allow displaying decimal d3 format
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179029501
looks good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by vgmartinez <gi...@git.apache.org>.
Github user vgmartinez commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179252591
I tested...LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/685
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179322637
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/685
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179662685
I have slightly different view, what I think original idea behind using
`var integerFormatter = d3.format(',.1d');`
was to display number like `123123123` to `123,123,123`, but by
`d3.round(d, 3)`
it looses its formatting, and in my suggestion if you use
`var integerFormatter = function(x){return d3.format(',')(d3.round(x, 3));};`
it retains formatting and have decimal precision as well, kind of hacky but it works.
Also its integerFormatter is referenced in paragraph.controller.js.html as well, do you think this should be changed as well ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179701947
+1 on format approach, but I think that shouldn't be called `integerFormatter` now since it works with decimal numbers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by minahlee <gi...@git.apache.org>.
Github user minahlee commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-180200250
@prabhjyotsingh your suggestion makes perfect sense. It's ready now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179664736
@prabhjyotsingh Doesn't look that hacky to me and preserve a nice formatting.
> Also its integerFormatter is referenced in paragraph.controller.js.html as well, do you think this should be changed as well ?
Didn't find any other references, could you post a link?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: [ZEPPELIN-595] Allow displaying d...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/685#issuecomment-179421920
This doesn't work well without a floating point
<img width="801" alt="screen shot 2016-02-03 at 11 34 06 am" src="https://cloud.githubusercontent.com/assets/710411/12794491/2f936124-ca6a-11e5-99b8-61100eb12ad0.png">
I would suggest to do `return d3.round(d, 3);` instead.
which would round to 3 digits maximum (when needed)
<img width="388" alt="screen shot 2016-02-03 at 11 36 34 am" src="https://cloud.githubusercontent.com/assets/710411/12794527/6815cb4a-ca6a-11e5-8cec-3ac995c30dd8.png">
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---