-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Common interface to interpret and execute API methods #1016
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
Changes from 1 commit
f2d268d
65f618d
7ef0404
edb6773
f50fcaa
60dd634
4bc8387
93b8253
43dc3d4
65d24f3
d2696e7
7db7f03
57fcf96
7db5f1f
26ad1ff
986b4dc
20ac69f
f993ed7
99d7c8e
f8c0094
6abec15
5df94a7
d35ee35
a554cad
e5a80ee
45717b1
52de9e4
0c40b02
df2d5bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,8 @@ module.exports = function updateMenusDefaults(layoutIn, layoutOut) { | |
// used to determine object constancy | ||
menuOut._index = i; | ||
|
||
menuOut._input.active = menuOut.active; | ||
|
||
contOut.push(menuOut); | ||
} | ||
}; | ||
|
@@ -48,7 +50,9 @@ function menuDefaults(menuIn, menuOut, layoutOut) { | |
var visible = coerce('visible', buttons.length > 0); | ||
if(!visible) return; | ||
|
||
coerce('active'); | ||
// Default to zero since active must be *something*: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case, we should add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good point. That works for me. I think that's best since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. The default is set. Removing this. I think this may have been a change that I thought was necessary while debugging but ultimately did not need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Hold on. The updatemenu There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay. Let me adjust on my end then. That makes sense. I guess it just bothered me that the first item is selected when the chart loads but the variable is not set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted updatemenus change and fixed sliders initialization + defaults to match the behavior. |
||
coerce('active', 0); | ||
|
||
coerce('direction'); | ||
coerce('type'); | ||
coerce('showactive'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -525,21 +525,21 @@ describe('attaching component bindings', function() { | |
destroyGraphDiv(gd); | ||
}); | ||
|
||
it('attaches bindings when events are added', function(done) { | ||
it('attaches and updates bindings for sliders', function(done) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard See tests. It checks:
|
||
expect(gd._internalEv._events.plotly_animatingframe).toBeUndefined(); | ||
|
||
Plotly.relayout(gd, { | ||
sliders: [{ | ||
// This one gets bindings: | ||
steps: [ | ||
{label: 'first', method: 'restyle', args: ['marker.color', 'red']}, | ||
{label: 'first', method: 'restyle', args: ['marker.color', 'blue']}, | ||
{label: 'second', method: 'restyle', args: ['marker.color', 'blue']}, | ||
] | ||
}, { | ||
// This one does *not*: | ||
steps: [ | ||
{label: 'first', method: 'restyle', args: ['line.color', 'red']}, | ||
{label: 'first', method: 'restyle', args: ['marker.color', 'blue']}, | ||
{label: 'second', method: 'restyle', args: ['marker.color', 'blue']}, | ||
] | ||
}] | ||
}).then(function() { | ||
|
@@ -577,4 +577,57 @@ describe('attaching component bindings', function() { | |
expect(gd._internalEv._events.plotly_animatingframe).toBeUndefined(); | ||
}).catch(fail).then(done); | ||
}); | ||
|
||
it('attaches and updates bindings for updatemenus', function(done) { | ||
expect(gd._internalEv._events.plotly_animatingframe).toBeUndefined(); | ||
|
||
Plotly.relayout(gd, { | ||
updatemenus: [{ | ||
// This one gets bindings: | ||
buttons: [ | ||
{label: 'first', method: 'restyle', args: ['marker.color', 'red']}, | ||
{label: 'second', method: 'restyle', args: ['marker.color', 'blue']}, | ||
] | ||
}, { | ||
// This one does *not*: | ||
buttons: [ | ||
{label: 'first', method: 'restyle', args: ['line.color', 'red']}, | ||
{label: 'second', method: 'restyle', args: ['marker.color', 'blue']}, | ||
] | ||
}] | ||
}).then(function() { | ||
// Check that it has attached a listener: | ||
expect(typeof gd._internalEv._events.plotly_animatingframe).toBe('function'); | ||
|
||
// Confirm the first position is selected: | ||
expect(gd.layout.updatemenus[0].active).toBe(0); | ||
|
||
// Modify the plot | ||
return Plotly.restyle(gd, {'marker.color': 'blue'}); | ||
}).then(function() { | ||
// Confirm that this has changed the slider position: | ||
expect(gd.layout.updatemenus[0].active).toBe(1); | ||
|
||
// Swap the values of the components: | ||
return Plotly.relayout(gd, { | ||
'updatemenus[0].buttons[0].args[1]': 'green', | ||
'updatemenus[0].buttons[1].args[1]': 'red' | ||
}); | ||
}).then(function() { | ||
return Plotly.restyle(gd, {'marker.color': 'green'}); | ||
}).then(function() { | ||
// Confirm that the lookup table has been updated: | ||
expect(gd.layout.updatemenus[0].active).toBe(0); | ||
|
||
// Check that it still has one attached listener: | ||
expect(typeof gd._internalEv._events.plotly_animatingframe).toBe('function'); | ||
|
||
// Change this to a non-simple binding: | ||
return Plotly.relayout(gd, {'updatemenus[0].buttons[0].args[0]': 'line.color'}); | ||
}).then(function() { | ||
// Bindings are no longer simple, so check to ensure they have | ||
// been removed | ||
expect(gd._internalEv._events.plotly_animatingframe).toBeUndefined(); | ||
}).catch(fail).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.
Hmm. Why do we need this?
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.
Without this, there's an intermediate state in which this value is updated when changes occur, but is
undefined
before that.I ran into this while testing: I checked this to see if the external value was updated when component interactions occurred. That worked fine, but before changes were made, the value was
undefined
, which was perhaps desired but did not reflect the actual state.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.
Removed this line.