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

Added a series 'zero' option to control automatic scaling. #911

Merged
merged 7 commits into from Jan 11, 2013

Conversation

dnschnur
Copy link
Member

Filled plot types, like areas and bars, convey information through size. They traditionally start from zero, because starting from a different value changes the amount of filled space, thereby distorting the value of the data. In some contexts, however, the fill is used in more of a decorative sense, and in this case many users (#316, #529, #856, and others) expect to see exactly the same behavior as an unfilled line plot.

Purists would argue that this is simply wrong, and the only correct solution is to use a different plot type, but I don't believe that Flot should pass such judgement. Since neither behavior is 'correct', I propose adding an option to pass the choice on to users.

The option is called zero, and applies to lines and bars series. It defaults to the current behavior, where zero is true for bars and filled lines, otherwise false. I then gave the datapoint format a new option called autoscale, which controls whether it is considered when auto-calculating the axis min/max. The zero option controls the value of autoscale for the dummy-point used to represent the bottom of a bar or area.

This pull request is a draft, for review and comment. I'll make updates to the API and changelog once the implementation is finalized. Some unresolved questions/issues are:

  • Currently the option is specific to lines and bars; should it be made global? This would be more consistent, but on the other hand it's simply not applicable to pie charts and many other plot types.
  • On lines, the option currently applies even when 'fill' is false. This has no negative effects, but might again be inconsistent/confusing.
  • The categories plugin duplicates some of the axis auto-scale code. The plugin enhancements that we're planning to do for 0.9 would take care of this, but for now I think I'll have to replicate my changes to the duplicate code in categories.

Area and bar plots normally use a minimum of zero, since their purpose
is to show size, and using an auto-scaled minimum distorts the plot's
meaning.  But this behavior is undesirable in cases where the plot type
is used in more of a decorative sense.

The zero option provides a way to control this behavior.  It defauls to
true for bars and filled lines.
Added a format option 'autoscale' that controls whether the given point
is considered when determining an automatic scale.

The lines & bars 'zero' option controls whether autoscale is set on the
dummy point that is inserted to create the series lower-bound.
@markrcote
Copy link
Contributor

I like this idea, and I think you've got a simple and straightforward approach.

It's perfectly fine to have that option restricted to lines and bars. There are already other options that are specific to particular plot types, and, as you say, this option doesn't apply to a lot of other plots. It can be added to others if needed, especially since you split this up into the lines/bars zero option and the format autoscale option.

I do however think it is a bit confusing that the option is applied even when 'fill' is false. I think down the road, if someone is working on that area in the code, it would probably take some digging to figure out why it is set when it has no effect (admittedly I can't think of such a situation, but you never know). A simple comment should suffice if you don't want to rework the expression.

I also think it's fine to have a bit of duplicated code if there are definite plans to rework it. Again, maybe add a comment to justify it.

Finally, just a little nit: you added a block in the new coding style, but pretty much all of jquery.flot.js is still in the old style. Rather than mix styles, I think we should stick with the old style until such time as we convert the whole file to the new style.

@ghost ghost assigned dnschnur Dec 31, 2012
@OleLaursen
Copy link
Member

Back from holidays - seems good to me too, it's been a long-standing problem. Agree with Mark's comments.

This also includes a tweak to zero's default.  Previously zero only
received a value if lines were visible; now it always receives a value,
matching the behavior of other contextual options.
Due to limitations in our plugin architecture, the categories plugin
duplicates code from Flot's core for adding a dummy point to snap the y
axis to zero.  We can get rid of this duplication in 0.9; for now we'll
just update the duplicate to match the change in core that introduced
the new 'zero' option.
@dnschnur
Copy link
Member Author

Here's the completed version, taking into account Mark's comments.

I realized, when I picked this up to finish it, that the 'autoscale' option isn't actually necessary. The only thing that the existing x and y options do is control scaling, so I could have achieved the same result simply by omitting them both from the dummy-point.

After some thought I decided to go ahead with adding a new option, though. The fact that x and y only control scaling is more of a temporary situation than the real design intent; at some point either we or a plugin author is likely to require knowledge of whether a value belongs to the x or y axis, and that's what x and y are really meant for. If we use this shortcut now we'll just run into compatibility issues later. The option is in any case more explicit and easier to understand and document.

I'll leave this up for a day or two before merging, in case anyone has comments, but it seems to work pretty well.

dnschnur added a commit that referenced this pull request Jan 11, 2013
Added a series 'zero' option to control automatic scaling.
@dnschnur dnschnur merged commit 29476b8 into flot:master Jan 11, 2013
@dnschnur dnschnur deleted the series-zero-option branch January 11, 2013 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants