Skip to content

Enable & disable predefined modebar buttons via layout and template#5660

Merged
archmoj merged 11 commits intomasterfrom
layout-modebar
May 18, 2021
Merged

Enable & disable predefined modebar buttons via layout and template#5660
archmoj merged 11 commits intomasterfrom
layout-modebar

Conversation

@archmoj
Copy link
Copy Markdown
Contributor

@archmoj archmoj commented May 14, 2021 •
edited
Loading

This PR moves attributes and supply default files into component/modebar, register modebar in core.js and then
resolves #5623 by introducing modebar.add and modebar.remove options.

Also this PR help simplify use of config.modeBarButtonsToRemove by accepting short name variations.
For example one could use zoomin instead of zoomIn2d to disable zoom in buttun.

@plotly/plotly_js

"add": {
    "flags": [
        "v1hovermode",
        "hoverclosest",
        "hovercompare",
        "togglehover",
        "togglespikelines",
        "drawclosedpath",
        "drawopenpath",
        "drawline",
        "drawrect",
        "drawcircle",
        "eraseshape"
    ]
},

"remove": {
    "flags": [
        "autoScale2d",
        "autoscale",
        "editInChartStudio",
        "editinchartstudio",
        "hoverCompareCartesian",
        "hovercompare",
        "lasso",
        "lasso2d",
        "orbitRotation",
        "orbitrotation",
        "pan",
        "pan2d",
        "pan3d",
        "reset",
        "resetCameraDefault3d",
        "resetCameraLastSave3d",
        "resetGeo",
        "resetSankeyGroup",
        "resetScale2d",
        "resetViewMapbox",
        "resetViews",
        "resetcameradefault",
        "resetcameralastsave",
        "resetsankeygroup",
        "resetscale",
        "resetview",
        "resetviews",
        "select",
        "select2d",
        "sendDataToCloud",
        "senddatatocloud",
        "tableRotation",
        "tablerotation",
        "toImage",
        "toggleHover",
        "toggleSpikelines",
        "togglehover",
        "togglespikelines",
        "toimage",
        "zoom",
        "zoom2d",
        "zoom3d",
        "zoomIn2d",
        "zoomInGeo",
        "zoomInMapbox",
        "zoomOut2d",
        "zoomOutGeo",
        "zoomOutMapbox",
        "zoomin",
        "zoomout"
    ]
}

@archmoj archmoj added this to the NEXT milestone May 14, 2021
@nicolaskruchten
Copy link
Copy Markdown
Contributor

nicolaskruchten commented May 14, 2021

I thought we'd settled on layout.modebar.(add|remove) ? No "buttonsto"

@archmoj
Copy link
Copy Markdown
Contributor Author

archmoj commented May 14, 2021

I thought we'd settled on layout.modebar.(add|remove) ? No "buttonsto"

Renamed in aeb1626.

@nicolaskruchten
Copy link
Copy Markdown
Contributor

nicolaskruchten commented May 14, 2021

Thanks! Is implementing "remove" a lot of additional work?

@archmoj
Copy link
Copy Markdown
Contributor Author

archmoj commented May 14, 2021

Thanks! Is implementing "remove" a lot of additional work?

Maybe no. But the need for that is not clear? Is it to hide every possible button? Not these hidden by default ones?

@nicolaskruchten
Copy link
Copy Markdown
Contributor

nicolaskruchten commented May 14, 2021

For symmetry and yes, to be able to remove additional buttons like the zoom buttons for example or home/reset-axes

@nicolaskruchten
Copy link
Copy Markdown
Contributor

nicolaskruchten commented May 14, 2021

Basically a layout equivalent to config.modebarButtonsToRemove which gets used fairly often in the wild.

@archmoj archmoj changed the title Enable predefined shape drawing and hover modebar buttons via layout and template Enable & disable predefined modebar buttons via layout and template May 15, 2021
alexcjohnson
alexcjohnson reviewed May 17, 2021
View reviewed changes
Comment thread src/components/modebar/manage.js Outdated
var buttonsToAdd = context.modeBarButtonsToAdd
.concat(fullLayout.modebar.add.split('+'));
var buttonsToRemove = context.modeBarButtonsToRemove
.concat(fullLayout.modebar.remove.split('+'));
Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson May 17, 2021

Choose a reason for hiding this comment

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

Trying to think if there are any cases we need to explicitly deal with precedence of config over layout. That's the way we decided it should work, right?

If config adds a button that layout has asked to remove, it can add it via the full object, but the string versions aren't defined in the add section below.

And if config removes a button that layout has asked to add, since removal happens first it won't be available yet when it's expected to be removed, so it will appear anyway.

So it seems like these concat need to filter out layout items that are in the opposite context entry.

Copy link
Copy Markdown
Contributor Author

@archmoj archmoj May 17, 2021

Choose a reason for hiding this comment

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

Good call. Addressed in 3daa2f3.

alexcjohnson
alexcjohnson reviewed May 17, 2021
View reviewed changes
Comment thread src/components/modebar/attributes.js Outdated
].join(' ')
},
add: {
valType: 'flaglist',
Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson May 17, 2021

Choose a reason for hiding this comment

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

Using a flaglist here is convenient for validation, but it's different from the API we use for config, an array. Here we don't need to support the object form, which obviously wouldn't fit in a flaglist, I just wonder if ease of validation is sufficient reason to differ. @nicolaskruchten thoughts?

Copy link
Copy Markdown
Contributor

@nicolaskruchten nicolaskruchten May 17, 2021

Choose a reason for hiding this comment

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

I prefer an array, like in config

Copy link
Copy Markdown
Contributor Author

@archmoj archmoj May 18, 2021

Choose a reason for hiding this comment

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

Addressed in 5c17d6b and 3293097.

alexcjohnson
alexcjohnson approved these changes May 18, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 looks good. Great tests!

@archmoj archmoj merged commit d8a1654 into master May 18, 2021
@archmoj archmoj deleted the layout-modebar branch May 18, 2021 15:10
@archmoj archmoj mentioned this pull request Aug 16, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

@nicolaskruchten nicolaskruchten Awaiting requested review from nicolaskruchten

@wbrgss wbrgss Awaiting requested review from wbrgss

Assignees

No one assigned

Labels

feature something new

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

Add modebar buttons control into layout

3 participants

@archmoj @nicolaskruchten @alexcjohnson