-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Fixes for Bokeh path plots #763
Conversation
0da3d7c
to
e38fdbb
Compare
Just rebased the PR, now ready to merge. |
if isinstance(val, tuple): | ||
val = rgb2hex(val) | ||
data['color'] += [val for _ in range(len(eldata['xs']))] | ||
if len(set(data.get('color'))) == 1: |
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'm a little confused by this bit. In the loop, you populate data
with 'color'
but here you pop it off again?
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.
Yes, if all the paths are the same color there's no need to set it individually for each path. It'll simply be set once separately from the data.
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.
Ok, thanks for clarifying!
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'll get into the habit of adding more comments for non-obvious things like this.
Other than one comment (which is only asking for clarification) this looks fine and I am happy to merge. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes for hover tools on polygons and adding batching for Path plots. This PR is based off #762 and should be merged after that one.