Set unified hovermode via template #4669

archmoj · 2020-03-22T00:56:24Z

Addressing #4667 i.e. to set unified hovermode via template.
Commit 2c76d79 fixes the bug.
Other commits make helper functions to check hovermode.

@plotly/plotly_js

alexcjohnson · 2020-03-22T22:49:53Z

src/plots/cartesian/layout_defaults.js

@@ -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;


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

Some tests failed without the layoutIn fall back.

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.

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.

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);

});

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.

@alexcjohnson Good call. Revised in the commits below.

alexcjohnson · 2020-03-24T19:52:03Z

src/components/fx/helpers.js

@@ -12,13 +12,13 @@ var Lib = require('../../lib');

 // look for either subplot or xaxis and yaxis attributes
 // does not handle splom case
-exports.getSubplot = function getSubplot(trace) {


nonblocking, but at one point these names were useful in stack traces. That may or may not not be the case anymore, based on changes to our build and sourcemap processes... but historically that was the reason for names like this that are otherwise unused.

alexcjohnson · 2020-03-24T19:57:12Z

src/components/fx/layout_defaults.js

-    } else hovermodeDflt = 'closest';
-
-    var hoverMode = coerce('hovermode', hovermodeDflt);
+    var hoverMode = handleHoverModeDefaults(layoutIn, layoutOut, fullData);


We talked about short-circuiting this one, right? something like:

var hoverMode = layoutOut.hovermode; if (hoverMode === undefined) { hoverMode = handleHoverModeDefaults(layoutIn, layoutOut, fullData); }

Is that a possibility? Or would that not work for some reason?

hovermode and clickmode use special coerce function which returns previously coerced value.

plotly.js/src/components/fx/hovermode_defaults.js

Lines 14 to 20 in abf3a55

module.exports = function handleHoverModeDefaults(layoutIn, layoutOut, fullData) {

function coerce(attr, dflt) {

// don't coerce if it is already coerced in other place e.g. in cartesian defaults

if(layoutOut[attr] !== undefined) return layoutOut[attr];

return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);

}

alexcjohnson

Looks great. Just one perf comment #4669 (comment) but that's not blocking. Let's do it! 💃

archmoj added 3 commits March 21, 2020 19:59

enable unified hovermode via template

2c76d79

refactor check for unified hover modes

3c883ff

make a similar function in helpers to test x or y hover modes

9c07a91

archmoj added bug something broken status: reviewable labels Mar 22, 2020

archmoj added this to the v1.53.0 milestone Mar 22, 2020

alexcjohnson reviewed Mar 22, 2020

View reviewed changes

archmoj added status: in progress and removed status: reviewable labels Mar 24, 2020

archmoj added 4 commits March 24, 2020 13:22

make a separate function to coerce clickmode and hovermode

11a4e27

use coerce hovermode in cartesian defaults

3c47bf9

move hovermode coerce outside the loop

dc704ae

improve test for spikelines

abf3a55

archmoj added status: reviewable and removed status: in progress labels Mar 24, 2020

alexcjohnson reviewed Mar 24, 2020

View reviewed changes

alexcjohnson approved these changes Mar 24, 2020

View reviewed changes

archmoj merged commit efb54b2 into master Mar 24, 2020

archmoj deleted the set-unified-hovermode-via-template branch March 24, 2020 20:44

archmoj mentioned this pull request Mar 25, 2020

New rangebreaks and unified hover should work from templates #4667

Closed

snyk-bot mentioned this pull request Apr 23, 2020

[Snyk] Upgrade plotly.js from 1.52.1 to 1.53.0 mc-ireiser/vue-plotly#1

Open

snyk-bot mentioned this pull request May 1, 2020

[Snyk] Upgrade plotly.js-basic-dist from 1.52.3 to 1.53.0 libamtrack/web#583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set unified hovermode via template #4669

Set unified hovermode via template #4669

archmoj commented Mar 22, 2020

alexcjohnson Mar 22, 2020

archmoj Mar 22, 2020

alexcjohnson Mar 22, 2020

archmoj Mar 23, 2020

archmoj Mar 23, 2020

alexcjohnson Mar 23, 2020

archmoj Mar 24, 2020

alexcjohnson Mar 24, 2020

alexcjohnson Mar 24, 2020

archmoj Mar 24, 2020

alexcjohnson left a comment

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

	for(component in componentsRegistry) {
	_module = componentsRegistry[component];

	if(_module.supplyLayoutDefaults) {
	_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData);
	}
	}

	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);
	});

	// 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);
	}
	}

	module.exports = function handleHoverModeDefaults(layoutIn, layoutOut, fullData) {
	function coerce(attr, dflt) {
	// don't coerce if it is already coerced in other place e.g. in cartesian defaults
	if(layoutOut[attr] !== undefined) return layoutOut[attr];

	return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);
	}

Set unified hovermode via template #4669

Set unified hovermode via template #4669

Conversation

archmoj commented Mar 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment