Skip to content

[ZEPPELIN/1356] The graph legend truncates at the nearest period (.) in its grouping#1351

Closed
Peilin-Yang wants to merge 2 commits intoapache:masterfrom
Peilin-Yang:ypeilin/ZEPPELIN-1356
Closed

[ZEPPELIN/1356] The graph legend truncates at the nearest period (.) in its grouping#1351
Peilin-Yang wants to merge 2 commits intoapache:masterfrom
Peilin-Yang:ypeilin/ZEPPELIN-1356

Conversation

@Peilin-Yang
Copy link
Contributor

@Peilin-Yang Peilin-Yang commented Aug 22, 2016

What is this PR for?

Fix the issue: in line graph if user uses the numbers that contains period(.), e.g. 3.14 in the groups the legend will only show 3 instead of 3.14.

What type of PR is it?

[Bug Fix]

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1356

How should this be tested?

Localhost test with screenshot

Screenshots (if appropriate)

Before
screen shot 2016-08-25 at 6 51 36 pm
After
screen shot 2016-08-25 at 6 45 19 pm

Questions:

  • Does the licenses files need update? No.
  • Is there breaking changes for older versions? No.
  • Does this needs documentation? No.

@bzz
Copy link
Member

bzz commented Aug 26, 2016

Looks great to me, thank you for taking care @Peilin-Yang !

CI is agree, merging to master, if there is no further discussion. (Please let me know if you think it should be part of 0.6.2 as well)

@corneadoug
Copy link
Contributor

@bzz I'd like to review it first

@Peilin-Yang
Copy link
Contributor Author

@bzz is that possible to apply this to 0.6.0-SNAPSHOT. We are still using that version in Twitter:)

@bzz
Copy link
Member

bzz commented Aug 26, 2016

@Peilin-Yang make sense, have updated jira fix-for version.

@corneadoug absolutely, please take your time for review.

@corneadoug
Copy link
Contributor

Tested LGTM, @Peilin-Yang Can you just remove the comment in that change?

@Peilin-Yang
Copy link
Contributor Author

@corneadoug sure, will do and push

@prabhjyotsingh
Copy link
Contributor

Can you take care of the trailing decimal as well ?

screen shot 2016-08-26 at 12 08 51 pm

@corneadoug
Copy link
Contributor

@prabhjyotsingh which trailing decimal and what is supposed to be the result?

@prabhjyotsingh
Copy link
Contributor

IMO, instead of UI showing "1.41234312421341234213" it should get truncated/ceil/floor to say not more than 3 places of decimal.

@corneadoug
Copy link
Contributor

@prabhjyotsingh It is already, that's why there is 1.412 shown in the tooltip.
The other one is the group name.

The original purpose of this PR is that in case of the group name we should not remove what is after the coma.

To give a non numerical example:

Before the PR

Group Name would go from hello.world -> hello in the legend

After the PR

Group Name would be shown as hello.world in the legend

@prabhjyotsingh
Copy link
Contributor

IMO shouldn't it be the other way, in tool tip, show the whole number without truncate, generally not to huge number.
Otherwise LGTM.

@corneadoug
Copy link
Contributor

@prabhjyotsingh I guess the tooltip value that is truncated can be the subject of another issue.

@Peilin-Yang
Copy link
Contributor Author

@prabhjyotsingh I agree with @corneadoug
truncation on numeric values in the legend to me is another improvement/feature.
What this PR does is to fix the bug of existing code.
Does that make sense to you?

@Peilin-Yang
Copy link
Contributor Author

@corneadoug should I make the CI build successful? I do not know the failure is because of my commit.

@Peilin-Yang Peilin-Yang reopened this Aug 26, 2016
@prasadwagle
Copy link

Thanks @Peilin-Yang! This problem and the one in #1363 were reported by Twitter users and I am glad we are fixing them.

The build failures are due to flaky tests, You may want to see what tests are failing and create jira issues for those if they don't exist already.

@corneadoug
Copy link
Contributor

corneadoug commented Aug 26, 2016

Ci failure is unrelated (Selenium tests are green, Builds with unit tests are green)
merging if there is no more discussions

@asfgit asfgit closed this in 11bdd71 Aug 29, 2016
asfgit pushed a commit that referenced this pull request Aug 29, 2016
…in its grouping

### What is this PR for?
Fix the issue: in line graph if user uses the numbers that contains period(.), e.g. 3.14 in the groups the legend will only show 3 instead of 3.14.

### What type of PR is it?
[Bug Fix]

### Todos

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1356

### How should this be tested?
Localhost test with screenshot

### Screenshots (if appropriate)
Before
<img width="1251" alt="screen shot 2016-08-25 at 6 51 36 pm" src="https://cloud.githubusercontent.com/assets/3334391/17991618/10e38f20-6af5-11e6-9399-196f7c3a325a.png">
After
<img width="1249" alt="screen shot 2016-08-25 at 6 45 19 pm" src="https://cloud.githubusercontent.com/assets/3334391/17991506/3225d5e0-6af4-11e6-8090-5ea2319444d3.png">

### Questions:
* Does the licenses files need update? No.
* Is there breaking changes for older versions? No.
* Does this needs documentation? No.

Author: Peilin Yang <peiliny@twitter.com>

Closes #1351 from Peilin-Yang/ypeilin/ZEPPELIN-1356 and squashes the following commits:

5dcf157 [Peilin Yang] remove comment
00527a5 [Peilin Yang] fix ZEPPELIN/1356

(cherry picked from commit 11bdd71)
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants