-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add custom_data argument to px functions #1764
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
Conversation
The way it works:
I did not implement deduplication because this will behave correctly even if the user provides the same column in |
How hard would it be to implement deduplication? |
basically it would be a matter of, when processing |
@nicolaskruchten no it wouldn't be hard, I can do it if you prefer. Much easier than debugging the CI problem with chart_studio anyway :-/. At the moment I'm adding nottest decorators to isolate the failing tests but of course it is only temporary |
hehe ok why don't you add the dedupe code and I'll try to crack this Chart Studio thing :) |
Regarding deduplication, on second thoughts it might be confusing to users. Let's say a user supplies |
So with |
I agree that putting the index in the 0-position shifts things over 1 and makes things a bit confusing, but the value of having the index always available in the same position is pretty high. |
this is why I suggested |
I wonder which solution (index at 0 and then increment other positions by 1, or keep positions and have index at -1) will confuse users less... Any thoughts @jonmmease ? |
and yes what you suggested is doable. |
Really in an ideal world we would use the My worry about putting it in the last position is that it's easy for a user to say "oh it's here in position N" and then hardcode |
Come to think of it, I don't think there's a restriction on the type of |
Deduplication done. As soon as the percy PR is merged I'll rebase to compare, and then we can converge on the format of customdata. |
How about this: let's just not automatically insert the index! Once #1767 is done, the user can simply say |
ok I removed the index. This should be ready, I'll work on a demo now. |
---------- | ||
args : dict | ||
args to be used for the trace | ||
trace_spec : dict |
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 is actually a NamedTuple
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
Code looks fine, but this is probably a case where a simple test case would be nice, especially during review, for stuff that's invisible like this :) |
@@ -111,6 +111,10 @@ | |||
colref_list, | |||
"Values from these columns appear as extra data in the hover tooltip.", | |||
], | |||
custom_data=[ | |||
colref_list, | |||
"Values from these columns are extra data, to be used in widgets or Dash callbacks for example.", |
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.
Maybe a note that this data is not user-visible, but is included in events emitted by the figure?
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.
done
@nicolaskruchten about a simple test, definitely yes. I still need to work on #1754 there were some version conflicts in particular to be solved. |
See https://gist.github.com/emmanuelle/fba6f0a2c387100ac1cc027b4b37e20a for a Dash app showcasing the use of |
The good thing is that Percy did not detect changes. |
Great! Yeah Percy is pretty good, sometimes throws false positives but not enough to want to ditch it :) So next steps on this PR are to land #1754 onto |
yes ! #1754 might be a bit tricky because of version management / conflicts, but let's see how it goes. |
💃 very nice, thank you :) |
closes #1751