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
Ruler interaction replacing Grid mouse listeners for comparative tooltips #499
Ruler interaction replacing Grid mouse listeners for comparative tooltips #499
Changes from 45 commits
42c8eaf
2b778ba
cf6eeb2
6c1bfa5
269ab2a
7480ff6
3c56aa0
be5e962
0aefa01
2026a0d
8283efc
bc65c85
f1fb498
5aec426
3718884
3dcd533
178ea29
192e273
ab66a64
b96da08
7121b63
11589b6
00d44af
ae93c80
7e46f2b
6f3ffb9
fe6457a
c1edff4
c190a2b
b576875
a643552
ff8b0aa
7214aa5
0247757
63687f9
ff48ac2
db3d1f0
fc4ff2a
b4d5bf3
ee1b29a
d63e6a4
7ef4e63
00c55e3
2095c04
7a17802
ced86b4
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.
nice 👍
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.
why use role to select instead of something like
svg.selectAll("circle.dot");
?if it also works, it seems more conventional in this codebase
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.
The use of that selector seemed to me more appropriate since we initially thought to enable the ruler on very different charts, which might have different graphic elements that need to be highlighted (for example, if I remember correctly there was an idea to have other shapes for the scatterplot other than circles - squares, triangles etc) that way we could have a single selector which includes all different svg elements.
But you're right, at the moment the
circle.dot
selector should work across all current scenarios 👍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.
do we need a backdrop?
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.
It is needed to attach mouse listeners
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.
that means we can get rid of the
grid.ts
event listeners for tooltip...?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 discussed this and while we don't think it would be good to remove listeners from
grid.ts
as it would affect other charts too (where ruler does not exist), what we can do is add the possibility to pass a boolean option to theGrid
class constructor to disable the event listeners for specific cases (right now only when ruler is active, which means only on the linechart)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.
just to recap after latest discussion, I'm now going to disable mouse listeners on grid and enable ruler interaction for every other chart and scale type 😅