Skip to content

Conversation

cpsievert
Copy link
Collaborator

No description provided.

@cpsievert cpsievert requested a review from jcheng5 October 4, 2018 15:58
@jcheng5
Copy link
Contributor

jcheng5 commented Oct 4, 2018

I don't think this is quite right. The plotly.js semantics are that whatever object is found at the property path, is completely replaced. Your semantics appear to be more like if there is a . in the property path, then do a merge. Here's an example that demonstrates the difference:

p <- plot_ly(x = 1:10, y = 1:10, symbol = I(15), marker = list(
  line = list(
    width = 5,
    color = "orange"
  )
))
p

# According to plotly.js semantics, this should revert the marker line
# back to its default color, but with this PR it's still orange
style(p, marker.line = list(width = 10))

Copy link
Contributor

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

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

^

return(trace)
}
trace[[path[1]]] <- trace[[path[1]]] %||% setNames(list(NULL), path[2])
trace[[path[1]]] <- trace_replace(trace[[path[1]]], path[-1], value)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is fine but I think you could've also just replaced both of these lines (73 and 74) with:

trace[[path[1]]] <- trace_replace(trace[[path[1]]] %||% list(), path[-1], value)

@cpsievert cpsievert merged commit 5c919be into master Oct 11, 2018
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