Skip to content

Set unified hovermode via template #4669

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 7 commits into from
Mar 24, 2020
Merged

Set unified hovermode via template #4669

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c76d79
enable unified hovermode via template
archmoj Mar 21, 2020
3c883ff
refactor check for unified hover modes
archmoj Mar 21, 2020
9c07a91
make a similar function in helpers to test x or y hover modes
archmoj Mar 21, 2020
11a4e27
make a separate function to coerce clickmode and hovermode
archmoj Mar 24, 2020
3c47bf9
use coerce hovermode in cartesian defaults
archmoj Mar 24, 2020
dc704ae
move hovermode coerce outside the loop
archmoj Mar 24, 2020
abf3a55
improve test for spikelines
archmoj Mar 24, 2020
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
5 changes: 3 additions & 2 deletions src/plots/cartesian/layout_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,9 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
handleTypeDefaults(axLayoutIn, axLayoutOut, coerce, defaultOptions);
handleAxisDefaults(axLayoutIn, axLayoutOut, coerce, defaultOptions, layoutOut);

var unifiedHover = layoutIn.hovermode && ['x unified', 'y unified'].indexOf(layoutIn.hovermode) !== -1;
var unifiedSpike = unifiedHover && axLetter === layoutIn.hovermode.charAt(0);
var hovermode = layoutOut.hovermode || layoutIn.hovermode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it not work with just layoutOut.hovermode? Do we need the layoutIn fallback for some specific case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests failed without the layoutIn fall back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is just unit tests (ie we're not running the entire supplyDefaults pipeline so earlier steps in that should have set layoutOut.hovermode were not called, then I'd rather change the test than include this fallback. If there's a case where the full pipeline does not behave correctly then this worries me, as that case presumably would not work with templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hovermode coerce with auto defaults depending on cartesian is

plotly.js/src/components/fx/layout_defaults.js

Lines 36 to 44 in 6de4c75

var hoverMode = coerce('hovermode', hovermodeDflt);
if(hoverMode) {
var dflt;
if(['x unified', 'y unified'].indexOf(hoverMode) !== -1) {
dflt = -1;
}
coerce('hoverdistance');
coerce('spikedistance', dflt);
}

called at the end of pipeline here:

plotly.js/src/plots/plots.js

Lines 1672 to 1678 in 6de4c75

for(component in componentsRegistry) {
_module = componentsRegistry[component];
if(_module.supplyLayoutDefaults) {
_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData);
}
}

I am not sure how to proceed if we want to remove layoutIn.hovermode fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On another note - here is test which is failing, if we remove layoutIn fallback:

plotly.js/test/jasmine/tests/hover_label_test.js

Lines 4037 to 4052 in 6de4c75

it('set smart defaults for spikeline in x unified', function(done) {
Plotly.newPlot(gd, [{y: [4, 6, 5]}], {'hovermode': 'x unified', 'xaxis': {'color': 'red'}})
.then(function(gd) {
expect(gd._fullLayout.hovermode).toBe('x unified');
var ax = gd._fullLayout.xaxis;
expect(ax.showspike).toBeTrue;
expect(ax.spikemode).toBe('across');
expect(ax.spikethickness).toBe(1.5);
expect(ax.spikedash).toBe('dot');
expect(ax.spikecolor).toBe('red');
expect(ax.spikesnap).toBe('hovered data');
expect(gd._fullLayout.yaxis.showspike).toBeFalse;
})
.catch(failTest)
.then(done);
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that test is failing without the layoutIn fallback, it means that spikes will not be properly enabled by hovermode set in the template, right?

So I guess the code here is called by the basePlotModules loop:

plotly.js/src/plots/plots.js

Lines 1639 to 1647 in 6de4c75

// base plot module layout defaults
for(i = 0; i < basePlotModules.length; i++) {
_module = basePlotModules[i];
// e.g. pie does not have a layout-defaults step
if(_module.supplyLayoutDefaults) {
_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData);
}
}

I think the easiest thing to do would be to move hovermode coercion here, right at the top of this function. That will involve making a special coerce function using fx/layout_attributes, but it will ensure the ordering is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson Good call. Revised in the commits below.

var unifiedHover = hovermode && ['x unified', 'y unified'].indexOf(hovermode) !== -1;
var unifiedSpike = unifiedHover && axLetter === hovermode.charAt(0);
var spikecolor = coerce2('spikecolor', unifiedHover ? axLayoutOut.color : undefined);
var spikethickness = coerce2('spikethickness', unifiedHover ? 1.5 : undefined);
var spikedash = coerce2('spikedash', unifiedHover ? 'dot' : undefined);
Expand Down
18 changes: 18 additions & 0 deletions test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4093,6 +4093,24 @@ describe('hovermode: (x|y)unified', function() {
.then(done);
});

it('unified hover modes should work for x/y cartesian traces via template', function(done) {
var mockCopy = Lib.extendDeep({}, mock);
delete mockCopy.layout.hovermode;
mockCopy.layout.template = {
layout: {
hovermode: 'y unified'
}
};
Plotly.newPlot(gd, mockCopy)
.then(function(gd) {
_hover(gd, { yval: 6 });

assertLabel({title: '6', items: ['trace 0 : 2', 'trace 1 : 5']});
})
.catch(failTest)
.then(done);
});

it('x unified should work for x/y cartesian traces with legendgroup', function(done) {
var mockLegendGroup = require('@mocks/legendgroup.json');
var mockCopy = Lib.extendDeep({}, mockLegendGroup);
Expand Down