Skip to content

lib function to interleave trace updates into a restyle #847

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

Closed
wants to merge 1 commit into from

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Aug 12, 2016

cc: @etpinard. This PR implements an unused library function that translates a frame-style update into a restyle-…um, style update. Goal: ease the burden on reviewing #802, so perhaps review here and I'll cherry-pick into that PR if it passes. It translates this:

[
  {x: [1], 'marker.color': 'red'},
  {x: [2], someprop: {enabled: true}}
]

into this:

{
  x: [[1], [2]],
  'marker.color': ['red', undefined],
  someprop: [undefined, {enabled: true}]
}

Caveat: There's some room for ambiguity if you specify marker.color and then overwrite with marker: {}. I'm a fan of strict sanity checking and good error reporting, but it's some extra work to either detect and throw an error or detect and resolve, so I'll hold off on this unless it seems necessary. At the very least, it's consistent. additional comment: How does restyle currently handle ambiguities? Unless we rigorously detect this case already, seems like this would all be the user's burden anyway.

Together with #844, this means frame updates can simply be interleaved and passed to Plotly.restyle. They are interleaved in order received, so that managing trace indices is not a concern of this function.

@rreusser rreusser force-pushed the trace-to-restyle-command branch 2 times, most recently from 5d6bbf3 to 127a4fd Compare August 12, 2016 00:37
var output = {};

for(i = 0; i < traces.length; i++) {
for(prop in traces[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

non- 🚫 , but we usually loop over object keys by first var keys = Object.keys(traces) and then looping of keys.

@etpinard
Copy link
Contributor

How does restyle currently handle ambiguities?

Here's what restyle does currently: http://codepen.io/etpinard/pen/oLmNPE

so I'll hold off on this unless it seems necessary

That's fine.

@etpinard
Copy link
Contributor

💃 once you're happy with it.

@rreusser rreusser force-pushed the trace-to-restyle-command branch from 127a4fd to 40344ff Compare August 12, 2016 15:14
@rreusser rreusser force-pushed the trace-to-restyle-command branch from 40344ff to e77a276 Compare August 12, 2016 15:15
@rreusser
Copy link
Contributor Author

Regarding ambiguities, I was referring to conflicts within a single statement, which seems to be order-dependent. See example.

Plotly.restyle('graph', {
  marker: {color: 'red', size: 10},
  'marker.color': 'blue'
})

@etpinard
Copy link
Contributor

etpinard commented Aug 12, 2016

I was referring to conflicts within a single statement, which seems to be order-dependent

Oh I see. The last key-value item ('marker.color': 'blue') should always override the previous (marker.color: 'red').

@rreusser did you find any cases where restyle didn't respect that logic?

@rreusser
Copy link
Contributor Author

I didn't find cases where it didn't obey it, but key order in iterated javascript objects, though likely nailed down pretty well in the specs, doesn't seem like the most friendly thing to confront people with on a regular basis. But it's user input anyway and it's well-defined, if weird, so I'm fine with it.

@rreusser
Copy link
Contributor Author

rreusser commented Aug 12, 2016

For reference:

ES5: 12.6.4 The for-in statement:

The mechanics and order of enumerating the properties (step 6.a in the first algorithm, step 7.a in the second) is not specified.

And from ES5: 15.2.3.14 Object.keys:

If an implementation defines a specific order of enumeration for the for-in statement, that same enumeration order must be used in step 5 of this algorithm.

That means there's no guarantee {marker: {color: 'red'}, 'marker.color': 'blue'} will have the same effect across different browsers. Practically speaking, keys are probably iterated in the ordered inserted. Though it's not a guarantee, making the input format potentially ambiguous. This is nothing new and probably has/never will present real problems, so am not pursuing this further at the moment.

@etpinard
Copy link
Contributor

etpinard commented Sep 1, 2016

@rreusser is this now part of #802 ?

@rreusser
Copy link
Contributor Author

rreusser commented Sep 1, 2016

@etpinard whoops. Didn't realize this wasn't already closed. It's not part, but instead managed to avoid the need for it by using the full supply defaults to apply the changes. So I think this is simply not needed, but there's a chance the code could come in handy if it does turn out to be necessary.

@rreusser rreusser closed this Sep 1, 2016
@etpinard etpinard deleted the trace-to-restyle-command branch November 15, 2016 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants