Skip to content
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

Cartesian dropline support #1461

Merged
merged 30 commits into from Apr 18, 2017
Merged

Cartesian dropline support #1461

merged 30 commits into from Apr 18, 2017

Conversation

rpaskowitz
Copy link
Contributor

@rpaskowitz rpaskowitz commented Mar 10, 2017

This PR adds support for droplines on cartesian graphs. It works with sub-plots and scatter/line/bar.

Some examples:
drop1
drop2
drop3
drop4

A new option is added to enable this behaviour (showDroplines), and it is disabled by default for backwards compatibility. Even if enabled, the chart must be on 'closest' hovermode and not 'compare'. Without this restriction there can be multiple points hovered simultaneously and that tended to make things confusing if there were multiple droplines.

A couple small things right now:

  • The droplines are drawn before the labels, but because the hover event fires on small moves, it can cause a redraw of the dropline causing it to be on top of the label. This should be pretty easily solved.
  • It would probably be better to draw the dropline to the secondary axis on the right, instead of the left, for series plotted on that axis. Some dropline implementations just draw the line full across which avoids that issue, but I prefer this styling. Getting the dropline to swap sides has the added benefit of making it much more clear to the user which axis they should be reading, which on charts with 2 is always a point of confusion at first glance. The bigger issue is what to do if there are even more axes off to the side, like on mocks/20.json, where the line may go to the plot edge and not the true axis, but where drawing the line on top of other axes to reach would look weird.

drop5

As usual, tests are a TODO. Please let me know if you'd prefer them sooner in the future. Also let me know if you'd prefer something other than a PR for a feature that may/may not be merged (which is why I skip tests off the bat)

@rpaskowitz rpaskowitz force-pushed the dropline branch 3 times, most recently from 9b14795 to 0c9962c Compare March 10, 2017 06:23
Draw droplines to the x and y axes on hover if the option is enabled and we're not on 'compare' hovermode.
@rpaskowitz
Copy link
Contributor Author

Related to this PR, but not exclusively, I think it may make sense to swap the hovertext anchor point based on if a data series is plotted on a right or left axis. With the droplines, it becomes more apparent, since often for a series plotted on a right axis the hovertext fully covers the horizontal dropline. Even without droplines though, having the hovertext to the right of the datapoint of a series which is plotted on a right axis makes it more difficult to visualize where it lands on the axis.

Specifically in the following block for the non-rotated case, add a check if the series is on the right axis, and if so, default to end and switch to start only in the case where the label would be off the left side of the chart.

        if(rotateLabels) {
            d.pos = htx;
            anchorStartOK = hty + dy / 2 + txTotalWidth <= outerHeight;
            anchorEndOK = hty - dy / 2 - txTotalWidth >= 0;
            if((d.idealAlign === 'top' || !anchorStartOK) && anchorEndOK) {
                hty -= dy / 2;
                d.anchor = 'end';
            } else if(anchorStartOK) {
                hty += dy / 2;
                d.anchor = 'start';
            } else d.anchor = 'middle';
        }
        else {
            d.pos = hty;
            anchorStartOK = htx + dx / 2 + txTotalWidth <= outerWidth;
            anchorEndOK = htx - dx / 2 - txTotalWidth >= 0;
            if((d.idealAlign === 'left' || !anchorStartOK) && anchorEndOK) {
                htx -= dx / 2;
                d.anchor = 'end';
            } else if(anchorStartOK) {
                htx += dx / 2;
                d.anchor = 'start';
            } else d.anchor = 'middle';
        }

I mention it here because I thought I had a bug where I didn't think the horizontal dropline was drawn, when really it was just fully obscured.

My only concern with this is that if you have 2 data points near one another, but plotted on opposite series, it may seem odd to the user that the hovertext swaps positions so drastically.

@etpinard
Copy link
Contributor

Thanks for the PR @rpaskowitz !!!

I like this new functionality a lot. Though I'm thinking it should be via layout.hovermode instead of some config option. @alexcjohnson @cldougl thoughts?

@alexcjohnson
Copy link
Contributor

Yeah, I like it! We should harmonize this behavior with gl2d, which currently does exactly this - always, cannot be turned off, can it? But it uses solid black lines:

screen shot 2017-03-10 at 10 50 56 am

I can see this being a hovermode - that works nicely with not allowing it in compare mode, and also allows the end viewer to turn it on and off, not relying on the developer, since it'll show up in the modebar. Then maybe we can turn it off by default for gl2d as well.

Thoughts on colors? For robustness I kind of want to match the box/lasso select outline, which is alternating black/white dashes (drawn as solid white with dashed black on top of it) which probably isn't the prettiest but it always shows up, no matter what data or background it's drawn against.

re: axes off to the side ("free" axes) - perhaps we can just add a tick or arrow on the axis itself but not draw the whole line over to it (wherever it may be), just draw the line to the edge of the plot area? Same goes for subplots that share an axis, we probably don't want the line to go all the way across the other subplot, just add a mark of some sort on the relevant axis.

re: flipping the default label orientation for series drawn against the right axis: I think that's a great idea! The hover label then functions as an arrow pointing toward that axis. There's a chance people will find this less intuitive, as the label is positioned toward the wrong axis, even if it's pointing toward the correct one... but the benefits seem greater so lets do it. A thought on implementation: if we did this based on the pixel position of the axis relative to the hovered-on point, rather than on the side, then if you put a free axis over the middle of the plot, the labels would flip as you cross the axis, which seems like it would be a cool & useful effect - and you'd automatically get it right for free axes off the edge like mocks/20.json

@rpaskowitz
Copy link
Contributor Author

Though I'm thinking it should be via layout.hovermode instead of some config option.

On one hand, I like using hovermode since it untangles the fact that this only works on specific hovermodes. On the other hand though, today hovermode really impacts what triggers a hover, and what data is included in the hover events, and not what is shown when a hover is triggered. I think that definition highlights why I struggle to name a new hovermode for this, since it would basically be closest_dropline, where apart from the UI aspect, it behaves identically to closest. Just as today there's no way to use the hovermode to control the hovertext...

...I'm picturing something like a hoverinfo (except not hoverinfo, since that controls what is in the hovertext) that could be set to text, dropline or text+dropline to control whether the current hovertext is shown at all, and this new dropline functionality. Currently you can't subscribe to hover events without the hovertext being shown, so this would provide a nice way to disable that (as opposed to nuking it in CSS or the DOM) as well as add this new functionality. What this doesn't solve is that droplines would still only be shown on closest hovermode.

Thoughts on colors? For robustness I kind of want to match the box/lasso select outline, which is alternating black/white dashes (drawn as solid white with dashed black on top of it) which probably isn't the prettiest but it always shows up, no matter what data or background it's drawn against.

I prefer the look with colors, but understand that it can cause some comprehension issues in certain scenarios. In others though I think it lends clarity and provides easier comparison across series. So far in my testing the colors look pretty good, even with some dense data series. I'd might suggest the white background behind the color, which should increase visibility while still keeping some indicator of the series?

perhaps we can just add a tick or arrow on the axis itself but not draw the whole line over to it

I could picture a dot/circle on the axis the goes slides along the axis to help make this clear. I'd still likely want to keep the line within the plot/subplot because a second convenience with droplines is not just finding the line on the axis, but also comparing data points of the same or other series with a selected on ("where we higher or lower 7 days ago?")

if we did this based on the pixel position of the axis relative to the hovered-on point

I'll need to unpack this a bit and learn a bit more about "free" axes, but I see the value of the generic implementation that also handles cases like an axis in the middle.

We should harmonize this behavior with gl2d

I stumbled across the droplines on gl2d after implementing this and had a moment of "is it somehow using my implementation from cartesian but failing to apply stroke?". Once we settled on behavior to enable and styling, I can take a look at the gl2d implementation, but it could take longer since I've not done any WebGL before.

@alexcjohnson
Copy link
Contributor

today hovermode really impacts what triggers a hover, and what data is included in the hover events, and not what is shown when a hover is triggered.

Good point - @etpinard is right that this shouldn't go in config, but we should make it another attribute, independent of hovermode, and give it a modebar icon that's only available in closest mode so viewers can toggle this.

This also has an analog in gl3d, where each axis independently has an attribute showspikes - along with spikesides for secondary lines, and spikethickness and spikecolor to style them. I can't say I'm that excited about the name "spikes" but we do already have it so using a different name could be confusing... That said, unlike 3D I don't think we need to allow this on a per-axis basis, a global setting seems fine. And maybe sometime we will want to enable styling like color and thickness but lets omit that for now.

I'd might suggest the white background behind the color, which should increase visibility while still keeping some indicator of the series?

Ah, that's nice - we could use the same coloring as the hover label: light colors get a black contrast, dark colors get a white contrast.

I could picture a dot/circle on the axis the goes slides along the axis to help make this clear. I'd still likely want to keep the line within the plot/subplot because a second convenience with droplines is not just finding the line on the axis, but also comparing data points of the same or other series with a selected on ("where we higher or lower 7 days ago?")

Oh yes - I wasn't suggesting getting rid of the line within the subplot... just not extending it outside the subplot if the axis isn't at the edge of the subplot. But the point about using this line for comparison is interesting - does this mean we should actually extend these lines the full width of the subplot, not just toward the axis?

I can take a look at the gl2d implementation, but it could take longer since I've not done any WebGL before.

Doesn't have to be in this PR, nor does it need to be your job unless you're itching to play with webGL. I just want to make sure that whatever we do here will also work for webGL when we get to it.

@etpinard
Copy link
Contributor

That said, unlike 3D I don't think we need to allow this on a per-axis basis, a global setting seems fine

I disagree here. I kinda like the idea of adding showspikes to every axis object. I can image someone only wanting spikes on the x (or y) axis of a given subplot. We should make sure that the xaxis.showspikes description mentions that it only has an effect when layout.hovermode is set to 'closest'.

And maybe sometime we will want to enable styling like color and thickness but lets omit that for now.

Agreed. No need to do this in this PR.

But the point about using this line for comparison is interesting - does this mean we should actually extend these lines the full width of the subplot, not just toward the axis?

For example, on a stacked/coupled subplot layout:

image

I think, yes, when xaxis.showspikes is turned on it would be nice to make the spikes reach the x-axis at the bottom. Now, maybe this isn't as obvious for other subplot layouts like:

image

here with yaxis.showspikes: true should the spikes reach the y axes with non-default position settings? I'm not sure. In this case, the spikes would overlap one or several y-axis title(s).

@rpaskowitz
Copy link
Contributor Author

rpaskowitz commented Mar 11, 2017

I think, yes, when xaxis.showspikes is turned on it would be nice to make the spikes reach the x-axis at the bottom. Now, maybe this isn't as obvious for other subplot layouts like:

Oh yes - I wasn't suggesting getting rid of the line within the subplot... just not extending it outside the subplot if the axis isn't at the edge of the subplot.

Here's what it looks like with dots on the axes. The dropline will draw the the edge of the plot area, but the dot will appear on whichever axis the series is associated to.

Ah, that's nice - we could use the same coloring as the hover label: light colors get a black contrast, dark colors get a white contrast.

This example also has a white line behind the colored line, which is also a pixel wider on each side. I'm not sure the contrast is needed for alternating colors because the backgrounds purpose is to make the line visible on top of other series, not necessarily visible on top of a series of the same color. Since there could be any number of other colors to differentiate from, alternating backgrounds may not help. I think if the background were black, it would stick out really poorly on a chart with a white background. (that being said, if the chart had a black or dark background, this one would stick out, so score another point for configurability - unless we wanted to use the chart base color to programatically determine the dropline background).

drop6

@rpaskowitz
Copy link
Contributor Author

rpaskowitz commented Mar 11, 2017

where each axis independently has an attribute showspikes

I kinda like the idea of adding showspikes to every axis object.

But the point about using this line for comparison is interesting - does this mean we should actually extend these lines the full width of the subplot, not just toward the axis?

Perhaps we could resolve this stuff by making showspikes configurable with options like false (off), axis on to plot to the data axis only, true to draw both ways. If it's configurable per axis someone could do true for the y axis to get the line all the way across, but keep the a axis at axis. Alternatively this could be done like hoverinfo where we could pack in whether lines are drawn to the data axis, entirely, and if the dot is drawn on the axis line.

@rpaskowitz
Copy link
Contributor Author

Took a re-read, but

I think, yes, when xaxis.showspikes is turned on it would be nice to make the spikes reach the x-axis at the bottom

makes sense to me now. It's certainly possible to draw all the way to the axis (I just did it accidentally), so maybe it would be another configurable option in the showspikes configuration to have the line drawn all the way to the axis for the cases where one knows it's not going to do something ugly like draw over other axes.

@rpaskowitz
Copy link
Contributor Author

Took a re-read, but

I think, yes, when xaxis.showspikes is turned on it would be nice to make the spikes reach the x-axis at the bottom

makes sense to me now. It's certainly possible to draw all the way to the axis (I just did it accidentally), so maybe it would be another configurable option in the showspikes configuration to have the line drawn all the way to the axis for the cases where one knows it's not going to do something ugly like draw over other axes.

I think this is actually a non-issue. Between the two screenshots are two different scenarios. In the first example, mocks/stacked_coupled_subplots, the 3 plots are all sharing a single x-axis, they don't have different free axes, while in mocks/multiple_axes_multiple and mocks/20 there are some series plotted on free axes. In the case of shared axes, we can simply draw the line directly to the axis. It is in the free case where we don't want to do that, and draw instead to the plot area.

I'm holding off for some more changes before pushing code, but here are some examples of my updated implementation:
stacked_coupled_subplots -- shared x-axis
drop_shared_x

stacked_subplots_shared_yaxis -- shared y-axis
drop_shared_y

20 -- free y-axes
drop_free_y

shared_axes_subplots -- shared x and y axes. The droplines really helped me understand this chart, unrealistic though it may be, except if someone were trying to make some sort of clever infographic venn like diagram, since at a glance I wasn't sure which sub-plots were using which axes, but it was very clear with the droplines.
drop_shared_xy

My implementation still needs to be updated to handle free x axes, since I'm only handling y right now. And I need to get rid of triple nested ternaries.

An interesting discovery is that while in 'compare' mode, the axis value callouts in shared axis scenarios are arguable in the wrong spot since they seem to be being positioned at the bottom of the y axis instead of on the x axis (which is basically what the earlier implementation of droplines was doing, so I imagine a similar fix is straightforward):
compare_marker

Added axis layout option for showspikes
Added background behind droplines
Added axis indicator marker, including on free anchored axes
Always draw dropline to chart limit, including for shared axes - not drawing all the way to free anchored axes
@rpaskowitz
Copy link
Contributor Author

Most points addressed in latest commit.

TODO:

  • Decide if we want either dropline to not just go to the axis the value relates to, but full across the chart. Could be configurable as part of showspikes.
  • Decide if we we need the background line behind the colored line to change based on chart and/or line color.
  • Decide if a modebar is needed to enable/disable this behavior (I'm not 100% on it being a modebar interaction since it wouldn't be a simple on/off, as it's per-axis, including individual axes when there are subplots, and it may not be desirable at all for certain charts - I think it may be best left to the chart designer - one example, x-axis spikes are pretty unnecessary for bar charts).
  • Decide if swapping the hovertext position based on axis position should be done as a part of this PR (my feeling now is no, since the axis marker mostly resolves the visibility issue that the problem previously exacerbated, and it's independent)
  • Tests, naturally

@alexcjohnson
Copy link
Contributor

@rpaskowitz thanks for all the great examples and exploration of so many scenarios!

That said, unlike 3D I don't think we need to allow this on a per-axis basis, a global setting seems fine

I disagree here. I kinda like the idea of adding showspikes to every axis object. I can image someone only wanting spikes on the x (or y) axis of a given subplot. We should make sure that the xaxis.showspikes description mentions that it only has an effect when layout.hovermode is set to 'closest'.

Decide if a modebar is needed to enable/disable this behavior (I'm not 100% on it being a modebar interaction since it wouldn't be a simple on/off, as it's per-axis, including individual axes when there are subplots, and it may not be desirable at all for certain charts - I think it may be best left to the chart designer - one example, x-axis spikes are pretty unnecessary for bar charts).

True, bar charts don't benefit from a guide line down the bar, and in fact it could obscure things... But we could handle that based on trace type instead. I worry that few people will go to the trouble of understanding its implications fully enough to configure it per-axis, and at least for me personally I would really appreciate the ability to turn this effect on or off (ie via the mode bar). I suppose we could define a global layout.showspikes that's controlled by the mode bar and sets the default that per-axis values override... seems potentially confusing though.

Here's what it looks like with dots on the axes. The dropline will draw the the edge of the plot area, but the dot will appear on whichever axis the series is associated to.

Decide if we want either dropline to not just go to the axis the value relates to, but full across the chart. Could be configurable as part of showspikes.

I like the look when the line is drawn from the point to the axis, no matter where that axis is displayed (even if it's a free axis?), and your point that this helps explain multi-axis plots is a great observation. Does that mean we don't need the dot on the axis? I like that look too when the axis has a line, but it seems redundant if the line gets there, and I don't like it when the axis doesn't have a line or ticks - then it overlaps the tick labels. Might be better just to leave it off?

Decide if we we need the background line behind the colored line to change based on chart and/or line color.

This example also has a white line behind the colored line, which is also a pixel wider on each side. I'm not sure the contrast is needed for alternating colors because the backgrounds purpose is to make the line visible on top of other series, not necessarily visible on top of a series of the same color. Since there could be any number of other colors to differentiate from, alternating backgrounds may not help.

I still think it would be worth trying exactly the colors that the hover label gets. Maybe it will look a bit heavy on white-background charts when the contrast color goes to black, but it will guarantee contrast in any situation: what if you have light-colored scatter points either in a thick cloud over a dark background, or with a dark marker.line on a light background. In both cases the spike lines would disappear. You're right that in a plot with a jumble of small feature of many different colors this won't really differentiate itself locally, but I still think except in some truly pathological cases it will make a clear feature to guide the eye vertically or horizontally when you consider the whole chart. That said I feel like the extra width of the white (or contrasting) line - which would help for some of these really chaotic cases - is probably too wide and obscures too much, particularly if you're using the line to compare values across the plot.

Decide if swapping the hovertext position based on axis position should be done as a part of this PR (my feeling now is no, since the axis marker mostly resolves the visibility issue that the problem previously exacerbated, and it's independent)

Agreed, but might be nice to make an issue for it so we don't forget to revisit it later.

@rpaskowitz
Copy link
Contributor Author

rpaskowitz commented Mar 13, 2017

Leaving aside the issue of per-legend, or chart-type, etc...fot now as I think that may need some internal plotly discussion. It mostly doesn't matter to me since I won't wind up using the modebar, and needing to enable globally, or per type, or per axis won't make too much of a difference.

I like the look when the line is drawn from the point to the axis, no matter where that axis is displayed (even if it's a free axis?), and your point that this helps explain multi-axis plots is a great observation. Does that mean we don't need the dot on the axis?

I'm not sure I follow all of this comment. I also like the line drawn from point to axis, but only to the graph edge in the case of a free axis (for the problem of having the line cross the other axes). I like the dot as an indicator on the free axes to help make the association, and while redundant when the axis on the chart-edge is the same as the series axis, I think the dot serves a purpose otherwise it will need to be a learned behaviour that "when the dot isn't shown, it means it's the axis on chart edge".

I like that look too when the axis has a line, but it seems redundant if the line gets there, and I don't like it when the axis doesn't have a line or ticks - then it overlaps the tick labels. Might be better just to leave it off?

I'll grant it does sometimes look a bit odd when there is no line on the axis, which happens not only when the line is disabled, but if the chart edge isn't at x = 0, so the line is offset from the chart edge. That's still the place at which the dropline itself will end, so I don't think it's too weird, and I think the placement could be adjusted a bit to make sure it doesn't cover the label text.

I think this may speak further to the need for something like the showspikes option to be like hovertext in accepting various options - this dot could be one of those configurable options - just as disabling tick markers is one, if this looks weird when there's no tick markers, then people should perhaps turn the dot off. My list of available spike options at this point would be toaxis, across, and marker. across would be 100% width, not just from the point, and would supersede toaxis.

I still think it would be worth trying exactly the colors that the hover label gets.

I can put together some samples with the different background and widths.

@etpinard
Copy link
Contributor

I worry that few people will go to the trouble of understanding its implications fully enough to configure it per-axis, and at least for me personally I would really appreciate the ability to turn this effect on or off (ie via the mode bar). I suppose we could define a global layout.showspikes that's controlled by the mode bar and sets the default that per-axis values override... seems potentially confusing though.

As discussed privately with @alexcjohnson, making this new mode bar button turn on or off the spikes would be pretty easy and shouldn't be a blocker for (x|y)axis.showspikes.

For example, we could do the same as in 3D where the mode bar "hover" handler turns off showspikes for all axes on 1st click and on 2nd click turns the original showspikes back on - see logic here.

Alternatively, we could add a true toggle button where the 1st click turns off showspikes for every axes, the 2nd click turns on showspikes for every axes and the current reset axes mode bar button would not only reset the axis range but also the spike settings back to their original values.

At the moment, I think the second option is the best (i.e. less confusing). Moreover, similar to @alexcjohnson 's idea, I think we should only show this new toggle spikes mode bar button when scatter traces are present on the graph.


  • Decide if we we need the background line behind the colored line to change based on chart and/or line color.

I'll let you and @alexcjohnson decide on what the best default spike color is. But I think we should at the very least expose spikecolor and spikethinkness attributes as part of this PR - on-par with the 3D spike attributes. Maybe we could also expose spikedash with the same possible values as the scatter line.dash.


I think this may speak further to the need for something like the showspikes option to be like hovertext in accepting various options - this dot could be one of those configurable options - just as disabling tick markers is one, if this looks weird when there's no tick markers, then people should perhaps turn the dot off. My list of available spike options at this point would be toaxis, across, and marker. across would be 100% width, not just from the point, and would supersede toaxis.

I think a flaglist attribute like spikemode with possible flags 'toaxis', 'marker' etc might be more plotly-esque. But I'm not 100% sure this setting needs to be exposed in this PR.

@rpaskowitz
Copy link
Contributor Author

But I think we should at the very least expose spikecolor and spikethinkness

I don't think this adds much work (spikedash too), so I'll just include it. Same goes for the flaglist, I don't think it is that big a deal, so may as well get it done (in my own projects, I find this stuff doesn't often get revisited after shipping, so sometimes better to just get it over with).

At the moment, I think the second option is the best

If I follow the current recommendations it would entail:

  • A per-axis showspike (plus associated color, thickness, dash, mode)
  • A per-cartesian-chart-type configuration (scatter X and Y true, line X and Y true, bar Y true, row X true, ...) of the above per-axis values
  • Allow manual override of the per-cartesian-chart-type configuration through axis layout options
  • A modebar that goes default -> (all off <-> all on)
  • The 'reset axis' modebar going back to default
  • default in both these cases meaning whatever the chart configuration happened to be when the spike toggle modebar button was hit (based on per-axis layout options)

If the 'per-axis-cartesian' defaults are desired, my main question would be what to do when there are mixed-series on a chart. Since the properties are currently being discussed as per-axis and not per-series, if there were a scatter+bar chart, then a line would be drawn to the x-axis for the bar as it would have been enabled by the scatter series' presence. I think resolving this would mean moving the spike settings to the series and having a flaglist for showspikes like x+y (I think it'd probably fine to keep the spike color, thickness, etc... the same for both axes for a given series, I can't picture wanting to draw those in different colors, and in fact, specifying those per-series may actually make more sense than specifying per-axis)

@etpinard etpinard added this to the v1.26.0 milestone Mar 22, 2017
@etpinard
Copy link
Contributor

@rpaskowitz @alexcjohnson status update on this thing? It would be nice to include it in next week's 1.26.0.

@rpaskowitz
Copy link
Contributor Author

I can likely get the spikecolor/thickness stuff done. Before tackling the rest in my last comment, if someone could confirm that I've properly described the functionality, I can get working on that too.

Completing tests would add the most uncertainty to wrapping this up, but if the functionality described is correct, implementing it shouldn't take too long, and I can start looking at the rest.

@alexcjohnson
Copy link
Contributor

A per-axis showspike (plus associated color, thickness, dash, mode)

👍

A per-cartesian-chart-type configuration (scatter X and Y true, line X and Y true, bar Y true, row X true, ...) of the above per-axis values
Allow manual override of the per-cartesian-chart-type configuration through axis layout options

I'd vote to omit this for now - just make the default off for all axes, and users can opt into it. We can revisit this later and decide if these warrant turning on by default, once we have the functionality available.

A modebar that goes default -> (all off <-> all on)
The 'reset axis' modebar going back to default

👍

default in both these cases meaning whatever the chart configuration happened to be when the spike toggle modebar button was hit (based on per-axis layout options)

I'd put this in the initial draw - ie alongside saveRangeInitial

@rpaskowitz
Copy link
Contributor Author

A per-cartesian-chart-type configuration (scatter X and Y true, line X and Y true, bar Y true, row X true, ...) of the above per-axis values
Allow manual override of the per-cartesian-chart-type configuration through axis layout options

I'd vote to omit this for now - just make the default off for all axes, and users can opt into it. We can revisit this later and decide if these warrant turning on by default, once we have the functionality available.

I think the issue in omitting this is the different expectations on different plot types, namely not wanting/needing a dropline on both axes for row/column charts. If we don't want to define them on the standard chart types, then I think the "allow manual override" remains from the earlier "per-axis showspike". From that perspective, this works for my needs, since we'll be able to configure it per-chart, and the only cases where it will be weird will be when someone takes a standard chart and uses the modebar to turn 'all on'.

The mode bar behavior for default -> (all off <-> all on) will now have default only be based on the initial chart rendering, which unless someone manually set up showspike on the individual axis, and with no defaults per chart, will be the same as all off.

I'll go ahead with that plan.

- toaxis always goes from the point to the axis, even for free axes
- across spans all counteraxes with subplots for this axis, and
  extends to the free axis if it exists
- fixed a bug with positioning after scrolling gd within a container
- also short-circuited calculations we don't need
@alexcjohnson
Copy link
Contributor

@etpinard @rpaskowitz I'm happy with this now - any comments on these commits before I merge?

@rpaskowitz
Copy link
Contributor Author

Gave the code a brief review, and played with a bunch of charts. Looks to be working as expected.

'width', 'autosize'].indexOf(ai) === -1) {
else if(['hovermode', 'dragmode'].indexOf(ai) !== -1 ||
ai.indexOf('spike') !== -1) {
flags.domodebar = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐎

@@ -629,7 +629,7 @@ function hover(gd, evt, subplot) {
}

// don't emit events if called manually
if(evt.target && !hoverChanged(gd, evt, oldhoverdata)) return;
if(!evt.target || !hoverChanged(gd, evt, oldhoverdata)) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Is this commit here neccessary as a result of rpaskowitz@b9f6ab1#diff-c3362661b0796b176be95487fa368e39R1488 ?

Did it fix a failing test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it did ;) 052417b

I'll note in case this confuses someone later: a bunch of the other tests in the hover_label_test suite look like they're providing pixel positions but are actually hitting this block that takes the middle of the x axis (and equivalently for y just below)

// a special type or at least a special coercion function, from the GUI
// you only get these values but elsewhere the user can supply a list of
// dash lengths in px, and it will be honored
values: ['solid', 'dot', 'dash', 'longdash', 'dashdot', 'longdashdot'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non- ⛔ , but It would be nice to reuse the list in traces/scatter/attributes.js

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned up a bunch of things in bd11567
@etpinard can you take a look at this? I tried to turn it into a regular enumerated attribute in cases where the manual dasharray isn't allowed (gl) and removed it where it's not supported at all (mapbox). I don't think this syntax is tested anywhere though... I suppose in principle we should provide mocks that use it for all the places it's used in SVG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍

spikemode: {
valType: 'flaglist',
flags: ['toaxis', 'across', 'marker'],
extras: ['none'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't spikemode: 'none' the same as showspikes: false?

@@ -173,6 +173,12 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

handleAxisDefaults(axLayoutIn, axLayoutOut, coerce, defaultOptions, layoutOut);

coerce('showspikes');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When showspikes: false, the attributes below shouldn't get coerced (as they don't do anything).

@etpinard
Copy link
Contributor

@alexcjohnson @rpaskowitz I'm a little concerned about #1461 (comment) as commit 69dc781 fixed manual hover event emitting, but no tests appeared to fail previous to that commit.

description: [
lineAttrs.dash,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously:
screen shot 2017-04-17 at 5 38 03 pm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much!

Plotly.plot(gd, data, layout).then(done);
});

fit('should emit events only if the event looks user-driven', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆘 fit 🆘

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, linter caught it...

Plotly.plot(gd, data, layout).then(done);
});

fit('should emit events only if the event looks user-driven', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test 🎉

@etpinard
Copy link
Contributor

💃 when tests ✅

I had to make a mock marker.line object in _fullData to keep legends happy,
but at least it doesn't respond to values in data.
@@ -26,14 +26,14 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
}

function coerceMarker(attr, dflt) {
var attrs = (attr.indexOf('.line') === -1) ? attributes : scatterAttrs;
// function coerceMarker(attr, dflt) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔪 🔪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants