Skip to content
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

sankey hover improvements #3150

Merged
merged 3 commits into from
Oct 25, 2018
Merged

sankey hover improvements #3150

merged 3 commits into from
Oct 25, 2018

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Oct 24, 2018

Addressing comments of @etpinard on PR #3096.

  • Replace .map call by Lib.repeat
  • Enable trace-level hover(info|label)

I also DRYed up some tests!

coerceLink('color', linkOut.value.map(function() {
return defaultLinkColor;
}));
coerceLink('color', Lib.repeat(defaultLinkColor, linkOut.value.length));
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks 🍻

@@ -21,6 +21,8 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
}

var hoverlabelDefault = Lib.extendDeep(layout.hoverlabel, traceIn.hoverlabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice touch 🖌️

var colorAttrs = require('../../components/color/attributes');
var fxAttrs = require('../../components/fx/attributes');
var domainAttrs = require('../../plots/domain').attributes;

var overrideAll = require('../../plot_api/edit_types').overrideAll;

var attrs = module.exports = overrideAll({
module.exports = overrideAll({
hoverinfo: plotAttrs.hoverinfo,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to extend the default hoverinfo declaration

hoverinfo: {
valType: 'flaglist',
role: 'info',
flags: ['x', 'y', 'z', 'text', 'name'],
extras: ['all', 'none', 'skip'],
arrayOk: true,
dflt: 'all',
editType: 'none',
description: [
'Determines which trace information appear on hover.',
'If `none` or `skip` are set, no information is displayed upon hovering.',
'But, if `none` is set, click and hover events are still fired.'
].join(' ')
},

with new flags. I'm thinking 'none', 'skip' and 'all'(the default) which I think should resolve @alexcjohnson 's concern #3096 (comment)

We should probably have a special description too to explain how trace hoverinfo can propagate to the node/link hoverinfo.

Copy link
Contributor Author

@antoinerg antoinerg Oct 25, 2018

Choose a reason for hiding this comment

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

@etpinard I'm having trouble setting hoverinfo.flags to an empty array [ ]. Running Plotly.PlotSchema.get().traces.sankey.attributes.hoverinfo always return 5 flags.

The following doesn't work:

var attrs = module.exports = overrideAll({
    hoverinfo: plotAttrs.hoverinfo,
    ...
})
attrs.hoverinfo.flags = []

The following also doesn't work:

var attrs = module.exports = overrideAll({
    hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
        flags: ['random']
    }),
    ...
})

although it replaces the first array element with 'random'.

I'm confused any help/pointer would be greatly appreciated 😕

Copy link
Collaborator

Choose a reason for hiding this comment

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

see #3058 (comment) - it's just a problem with the schema, the modules themselves are fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

the modules themselves are fine.

Right, gd._fullData[0]._module.attributes.hoverinfo.flags should have the new list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you guys for the quick reply! But won't this be an issue for the automatically generated documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure will 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

and already is - feel like tackling #3058, or at least that part of it?

Copy link
Contributor

@etpinard etpinard Oct 25, 2018

Choose a reason for hiding this comment

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

That, or remove hoverinfo from the global (plots/attributes.js) trace attributes.

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 I will look into #3058! I actually wanted to improve on plot.ly/javascript/reference so I guess that would be a good start for me since they're related :)

var attrs = module.exports = overrideAll({
module.exports = overrideAll({
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
flags: [],
Copy link
Contributor

@etpinard etpinard Oct 25, 2018

Choose a reason for hiding this comment

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

Nicely done, plotAttrs.hoverinfo already has the correct extras field

extras: ['all', 'none', 'skip'],

@etpinard
Copy link
Contributor

Nice job @antoinerg this PR is already worthy of a 💃

I'd say tackling #3058 or #3150 (comment) will deserve a PR of its own.

@antoinerg antoinerg merged commit c3a0118 into master Oct 25, 2018
@antoinerg antoinerg deleted the sankey-trace-hover branch October 25, 2018 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression this used to work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants