-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Enable & disable predefined modebar buttons via layout and template #5660
Conversation
I thought we'd settled on layout.modebar.(add|remove) ? No "buttonsto" |
Renamed in aeb1626. |
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? |
For symmetry and yes, to be able to remove additional buttons like the zoom buttons for example or home/reset-axes |
Basically a layout equivalent to config.modebarButtonsToRemove which gets used fairly often in the wild. |
…se exact and short names
src/components/modebar/manage.js
Outdated
var buttonsToAdd = context.modeBarButtonsToAdd | ||
.concat(fullLayout.modebar.add.split('+')); | ||
var buttonsToRemove = context.modeBarButtonsToRemove | ||
.concat(fullLayout.modebar.remove.split('+')); |
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.
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.
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.
Good call. Addressed in 3daa2f3.
src/components/modebar/attributes.js
Outdated
].join(' ') | ||
}, | ||
add: { | ||
valType: 'flaglist', |
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.
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?
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.
I prefer an array, like in config
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.
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 good. Great tests!
This PR moves attributes and supply default files into
component/modebar
, registermodebar
incore.js
and thenresolves #5623 by introducing
modebar.add
andmodebar.remove
options.Also this PR help simplify use of
config.modeBarButtonsToRemove
by accepting short name variations.For example one could use
zoomin
instead ofzoomIn2d
to disable zoom in buttun.@plotly/plotly_js