Skip to content

3D hover text fixes for date axes #1414

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 27, 2017
Merged

3D hover text fixes for date axes #1414

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bed43d1
use d2l to generate 3D hover label text
etpinard Feb 24, 2017
62ffd06
add test for 3D hover labels on non-linear axes
etpinard Feb 24, 2017
e9e0c83
improve scatter3d hover test
etpinard Feb 27, 2017
cbf1f86
fixup calendar on hover handling & add test
etpinard Feb 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/plots/gl3d/scene.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ function render(scene) {
}

function formatter(axisName, val) {
if(typeof val === 'string') return val;

var axis = scene.fullSceneLayout[axisName];
return Axes.tickText(axis, axis.c2l(val), 'hover').text;
var calendar = trace[axis._id.charAt(0) + 'calendar'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't we actually want to use the axis calendar for this? (which is what you get if you omit calendar as an arg to d2l)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if I understand correctly: hover labels should show dates with the calendar of the axes (as opposed to the calendar of the trace)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. That's what we do in 2D, and I think it makes sense, particularly when you're plotting different traces each with its own calendar, you want to be able to compare them within the system they were plotted on, irrespective of what the source data looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cbf1f86 - looks ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect - thanks for the test.


return Axes.tickText(axis, axis.d2l(val, 0, calendar), 'hover').text;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a mock with category data in 3D, though it does work:

Plotly.newPlot(gd,[{x:[1,2,3],y:['a','b','c'],z:['2014-01-01','2016-01-01','2015-01-01'], type:'scatter3d'}])

I think this code is compatible with category data, but can you include a test for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one here

https://github.com/plotly/plotly.js/pull/1414/files#diff-c406dc5b6d20f7f95206516178b5be36R148

should do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh perfect. My turn for new 👓

}

var oldEventData;
Expand Down
56 changes: 48 additions & 8 deletions test/jasmine/tests/gl_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe('Test gl plot interactions', function() {

describe('scatter3d hover', function() {

var node, ptData;
var ptData;

beforeEach(function(done) {
gd.on('plotly_hover', function(eventData) {
Expand All @@ -100,14 +100,18 @@ describe('Test gl plot interactions', function() {
delay(done);
});

it('should have', function() {
node = d3.selectAll('g.hovertext');
function assertHoverText(xLabel, yLabel, zLabel) {
var node = d3.selectAll('g.hovertext');
expect(node.size()).toEqual(1, 'one hover text group');

node = d3.selectAll('g.hovertext').selectAll('tspan')[0];
expect(node[0].innerHTML).toEqual('x: 140.72', 'x val on hover');
expect(node[1].innerHTML).toEqual('y: −96.97', 'y val on hover');
expect(node[2].innerHTML).toEqual('z: −96.97', 'z val on hover');
var tspan = d3.selectAll('g.hovertext').selectAll('tspan')[0];
expect(tspan[0].innerHTML).toEqual(xLabel, 'x val on hover');
expect(tspan[1].innerHTML).toEqual(yLabel, 'y val on hover');
expect(tspan[2].innerHTML).toEqual(zLabel, 'z val on hover');
}

it('should have', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't new with this PR, but this labeling is a little weird - it looks like you're including the message ('x val on hover' etc) as part of the test description, but doing that doesn't make it read out the right way either in the code or when the test fails.

The way I understand it, these tests should be described as:

describe('Noun', function() {
    it('makes a sentence when you include the "it"', function() {
        expect(true).toBe(false, 'more info if useful pinpoint the problem');
    });
});

Which, when it fails, will make a message that starts with a sentence:

Chrome 56.0.2924 (Mac OS X 10.12.3) Noun makes a sentence when you include the "it" FAILED
        Expected true to be false, more info if useful pinpoint the problem.
            <traceback>

So in this case I'd have expected it to be something like:

describe('gl plot interactions', function() {
    it('makes the right hover text and point data', function() {
        // I'm not sure if the messages add much, I typically only include them
        // when they include some variables either to determine which loop entry
        // we failed on or some input to the calculation that led to the failure
        // but having them doesn't detract from the test usability so you're
        // welcome to keep them if you find them useful
        expect(ptData.x).toBe('140.72');
    });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e9e0c83 - better?

assertHoverText('x: 140.72', 'y: −96.97', 'z: −96.97');

expect(Object.keys(ptData)).toEqual([
'x', 'y', 'z',
Expand All @@ -119,8 +123,44 @@ describe('Test gl plot interactions', function() {
expect(ptData.z).toEqual('−96.97', 'z val hover data');
expect(ptData.curveNumber).toEqual(0, 'curveNumber hover data');
expect(ptData.pointNumber).toEqual(2, 'pointNumber hover data');
});

Plotly.restyle(gd, {
x: [['2016-01-11', '2016-01-12', '2017-01-01', '2017-02']]
})
.then(function() {
mouseEventScatter3d('mouseover');
return delay;
})
.then(function() {
assertHoverText('x: Jan 1, 2017', 'y: −96.97', 'z: −96.97');

return Plotly.restyle(gd, {
x: [[new Date(2017, 2, 1), new Date(2017, 2, 2), new Date(2017, 2, 3), new Date(2017, 2, 4)]]
});
})
.then(function() {
mouseEventScatter3d('mouseover');
return delay;
})
.then(function() {
assertHoverText('x: Mar 3, 2017', 'y: −96.97', 'z: −96.97');

return Plotly.update(gd, {
y: [['a', 'b', 'c', 'd']],
z: [[10, 1e3, 1e5, 1e10]]
}, {
'scene.zaxis.type': 'log'
});
})
.then(function() {
mouseEventScatter3d('mouseover');
return delay;
})
.then(function() {
assertHoverText('x: Mar 3, 2017', 'y: c', 'z: 100k');
})
.then(done);
});
});

describe('scatter3d click events', function() {
Expand Down