-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bar | histogram lasso / select box dragmode #2045
Conversation
- use coordinates of middle of bar in the pos axis and outmost span in the size axis
- by reusing Bar.selectPoints 🎉
} | ||
else { | ||
x0 = xa.c2p(p0, true); | ||
x1 = xa.c2p(p1, true); | ||
y0 = ya.c2p(s0, true); | ||
y1 = ya.c2p(s1, true); | ||
|
||
// for selections | ||
di.ct = [(x0 + x1) / 2, y1]; |
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.
🆘 debatable logic alert 🆘
I chose to make a bar selected when the lasso/select polygon contains this point here which corresponds to the middle of the position coordinate at full size length.
Looking for 👍 s from @alexcjohnson @rreusser @monfera @chriddyp @cpsievert
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.
Huh, that's a good question. The way you did it makes sense from a data perspective - the endpoint on the size axis is really the salient position of the bar's data, which is why we put the hover label there. But as far as the metaphor of "grabbing an object" goes, the center might be more intuitive. Usually I go with the data perspective though, and it seems to me like that might be more useful ("select all bars above 100" etc) and I bet people will get used to it. So I'm a 👍 but curious to hear others' perspectives - particularly @cpsievert
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 seems reasonable (especially with a 1-to-1 mapping from the (input) data value to bar), but another important question is what data do you attach to the select event when a single bar represents an aggregation of multiple values? It seems important to provide options as to how you'd like to respond to the event, so I if it were me, I would emit the raw data, the aggregated data, and the selection region (on the data scale).
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.
so I if it were me, I would emit the raw data, the aggregated data, and the selection region (on the data scale).
Right now, we emit raw data and selection region. Adding the aggregated data is an interesting idea.
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.
And, maybe I should point out that, assuming the model doesn't allow for partially selecting a bar that represents multiple values, I would actually vote for the selection region to cover the entire bar in order for a selection to be made...
The problem with that though is that it could be confusing for most users if we implement two different selection criteria based on whether an aggregation is taking place
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 would offer a few arguments in favor of just keeping the "select the end" behavior:
- Some users do their own aggregation, making a
bar
trace that conceptually represents a histogram. What would we do in that case, introduce an option for whether to select the end or the whole thing? - It would be meaningless (or at least ambiguous) to disaggregate by selecting a portion of the bar. Which sample is on top vs on bottom?
- All our other selections work by collapsing the entity to a single point to be selected
- It makes it that much harder to select just the bars you want: you could still select all the small bars, but you wouldn't have any way to do the likely more common act of selecting all the largest bars.
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 would lean towards a consistent logic for bars & hists (and maybe the possibility of adding modes in the future). I think having a different behaviour across those two charts would cause more confusion than benefits across users, especially given how often I see users creating their own "histograms" with the bar
trace as @alexcjohnson mentioned above.
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.
More attributes are probably not a good idea, but perhaps the bar default would be data endpoints and the histogram default would be centroid. And if you dig deep enough to notice this subtlety, it'll lead to an attribute that can be toggled. If additional attributes had no cost, that'd be my preference, but additional attributes do have some small but nonzero cost (bundle size, documentation size, discoverability, maintenance, sprawl, configurability over good defaults, etc.) Realistically though, I would not actually expect the attribute to ever be toggled intentionally.
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 think having a different behaviour across those two charts would cause more confusion than benefits across users
I agree. I think there should be one simple and predictable way for this to work.
If the app developer really wants to allow users to select "partial bars" because it represents important aggregated data, then I'd argue that they should be more explicit about this in their UI and draw a rug plot below their histogram and allow users to select the exact partial sets of points that way. Or even a box plot with points or eventually a violin plot with points.
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 everyone for your input. I believe the keep-as-is side wins.
} | ||
} | ||
|
||
node3.selectAll('.point').style('opacity', function(d) { |
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.
test/jasmine/tests/select_test.js
Outdated
[0, 4.9, 0.371], [0, 5, 0.368], [0, 5.1, 0.356], [0, 5.2, 0.336], | ||
[0, 5.3, 0.309], [0, 5.4, 0.275], [0, 5.5, 0.235], [0, 5.6, 0.192], | ||
[0, 5.7, 0.145], | ||
[1, 5.1, 0.485], [1, 5.2, 0.40900000000000003], [1, 5.3, 0.327], |
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.
can we use toBeCloseTo
above in makeAssertPoints
and avoid 0.40900000000000003
etc?
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 in 3c73301
No more comments from my side - nicely done! 💃 |
Knocking out a few more items in #170