Skip to content

Issue 6. Add tootip - #41

Merged
stormpython merged 3 commits into
stormpython:masterfrom
nmqhoan:issue-6
Sep 26, 2016
Merged

Issue 6. Add tootip#41
stormpython merged 3 commits into
stormpython:masterfrom
nmqhoan:issue-6

Conversation

@nmqhoan

@nmqhoan nmqhoan commented Sep 15, 2016

Copy link
Copy Markdown
Collaborator

We updated the code based on your indications in PR #38
close #6

@stormpython stormpython mentioned this pull request Sep 15, 2016
@stormpython

Copy link
Copy Markdown
Owner

@nmqhoan thanks for the new pr. I will check this out over the weekend. But just out of curiousity, where is d3.tip used? I just did a cursory look through and couldn't find where it was being implemented.

@tuhp

tuhp commented Sep 16, 2016

Copy link
Copy Markdown

Hi @stormpython ,
This is redundant file. It does not use anywhere in the heatmap. It should be removed.
Thanks.

@nmqhoan

nmqhoan commented Sep 16, 2016

Copy link
Copy Markdown
Collaborator Author

@stormpython, thanks for reviewing the code. I removed the d3.tip.js and committed.

Comment thread public/heatmap_tooltip.css Outdated
height: auto;
padding: 5px;
z-index: 150;
background-color: #d3d3d3;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this can be changed to match the style in Kibana, which is:
fadeout(@gray-darker, 7%);
gray-darker: #222222

Comment thread public/tooltip_directive.js Outdated
@@ -0,0 +1,52 @@
var d3 = require("d3");

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I am guessing this file should be removed?

var text = g.selectAll('text.title')
.data([data]);

text.exit().remove();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why was the space here removed?

var text = d3.select(this).selectAll('text.' + cssClass)
.data(data);

text.exit().remove();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why was the space above removed?

Comment thread public/vis/index.js
selection.each(function (data) {
events.listeners(listeners);

layout.attr({

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why was the space above removed?

Comment thread public/lib/heatmap_controller.js Outdated

var title = d3.selectAll('text.title');
var value = d[key];
if(key.toUpperCase() === 'ROW')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

there should be a space between if and (

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same goes for the if statement below.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

They are also missing curly braces.

@stormpython stormpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Aside from a few syntax things, this pr looks good.

@stormpython

stormpython commented Sep 26, 2016

Copy link
Copy Markdown
Owner

LGTM! @nmqhoan thanks for getting this in.

@stormpython
stormpython merged commit 9e78176 into stormpython:master Sep 26, 2016
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.

Add tooltip

3 participants