Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
support template string on hover #3126
support template string on hover #3126
Changes from 31 commits
4c197e8
613c8f5
edf4795
dd62855
a86327a
1fde3aa
d263cce
040208f
3eaa3b9
0084efb
046d367
59083ad
266d43a
108b68a
eb1a94a
8214d36
bb97abe
22e3a9d
377ecba
bc6cbab
8c9ba5b
c767482
6d4c03a
8f440b0
0659d20
41ab464
37e40f9
8e8b75b
2015c57
aef48c0
a3058f4
3068d7d
a808883
522f744
c501b44
f89baca
369c9d6
8d95f05
1e4bd33
61a2da7
43a4cd9
70befe3
b6822e8
59bc981
f75f2e0
0654245
0ecd1c1
6f86522
7be804e
583eb97
14ac99f
1caf321
a78600a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Very interesting. I've never been a big fan of
hasOwnProperty
(probably because it's a long word to type out), but look at this:using http://jsbench.github.io/#1aebf699d73fb743127903c0b0a8bece
... so maybe we should start switching to
hasOwnProperty
in hot codepaths 🔥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.
We should append the
hovertemplate
descriptions with a list of flags (i.e. event data keys) available.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.
On a related note: I like that for some traces, like
pie
, we test that event data has the correct keys:plotly.js/test/jasmine/tests/pie_test.js
Lines 824 to 856 in ad937d2
We could potentially use the same array of field names for the test and for the description making sure we have one source of truth 🤔. This improvement could be done in another PR.
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.
Yeah, good call. We usually put things like that in "constants" files like this one:
https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/constants.js
That's up to you!
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 created a new function to check that event data contains a given list of keys: https://github.com/plotly/plotly.js/pull/3126/files#diff-4ddfa80f2c61e112d5587c91a961ab47
If it looks OK, I will dry up the code by listing the extra keys of a given trace into its
constants.js
file and then use that list to generate the description ofhovertemplate
attribute 🎉 !Let me know if that route looks fine to you.