-
-
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
Fix modeBarButtons issue #6177
Fix modeBarButtons issue #6177
Conversation
98e67de
to
c1b6ff2
Compare
src/components/modebar/manage.js
Outdated
@@ -8,6 +8,7 @@ var isUnifiedHover = require('../fx/helpers').isUnifiedHover; | |||
var createModeBar = require('./modebar'); | |||
var modeBarButtons = require('./buttons'); | |||
var DRAW_MODES = require('./constants').DRAW_MODES; | |||
var cloneDeep = require('lodash').cloneDeep; |
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.
Thanks very much for the PR.
Please note that lodash
is a dev-dependency.
And for various reasons (e.g. size & potential security issues) we are not interested to include that in our bundles.
Alternatively you could use
var extendDeep = require('../../lib').extendDeep;
and then
var customButtons = extendDeep([], context.modeBarButtons);
Or perhaps even better would be to use extendFlat
if possible?
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.
Fixed. Using the extendFlat function is not possible because the object has a nested structure.
Please add a test here: plotly.js/test/jasmine/tests/modebar_test.js Line 158 in 042742d
Also I appreciate if you draft Thank you! |
draftlogs/6177_fix.md
Outdated
@@ -0,0 +1 @@ | |||
- Fix modeBarButtons mutate the input, issue [[#1157](https://github.com/plotly/dash/issues/1157)] |
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.
- Fix modeBarButtons mutate the input, issue [[#1157](https://github.com/plotly/dash/issues/1157)] | |
- Fix custom modebar buttons mutate the input [[#6177](https://github.com/plotly/plotly.js/pull/6177)] |
src/components/modebar/manage.js
Outdated
@@ -44,7 +45,7 @@ module.exports = function manageModeBar(gd) { | |||
].join(' ')); | |||
} | |||
|
|||
var customButtons = context.modeBarButtons; | |||
var customButtons = extendDeep([], context.modeBarButtons); |
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.
This deep copy appears to be needed in the first case only.
I suggest you revert the change here and move deep copy inside fillCustomButton
function.
For example:
function fillCustomButton(inButtons) {
var customButtons = extendDeep([], inButtons);
...
Thanks very much for the PR. |
Fixes dash issue #1157
As @alexcjohnson wrote: