-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Conversation
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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); | |
}); |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} else hovermodeDflt = 'closest'; | ||
|
||
var hoverMode = coerce('hovermode', hovermodeDflt); | ||
var hoverMode = handleHoverModeDefaults(layoutIn, layoutOut, fullData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just one perf comment #4669 (comment) but that's not blocking. Let's do it! 💃
Addressing #4667 i.e. to set unified
hovermode
viatemplate
.Commit 2c76d79 fixes the bug.
Other commits make helper functions to check
hovermode
.@plotly/plotly_js