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

Centered axis #247

Merged
merged 8 commits into from Jan 11, 2017
Merged

Centered axis #247

merged 8 commits into from Jan 11, 2017

Conversation

parikhshiv
Copy link
Contributor

@parikhshiv parikhshiv commented Jan 4, 2017

Add new centeredAxis option for y-axis - focus is on dynamically centering y axis around data. Unlike default axis scaling, this option ignores the zeroAnchor option (meaning that the y axis will not be anchored at zero by default).

This PR also includes the changes from branch y-axis-scaling.

Here is a jsfiddle that illustrates the use of the centeredAxis option - http://jsfiddle.net/a5L7x0c5/1/

@parikhshiv parikhshiv mentioned this pull request Jan 4, 2017
@parikhshiv parikhshiv merged commit c08813f into master Jan 11, 2017
@parikhshiv parikhshiv deleted the centered-axis branch January 11, 2017 19:33
@narenranjit
Copy link
Contributor

@parikhshiv
Can you comment on the new options added? This'll then need to be assigned to molly for documentation purposes.

Also, as a general rule of thumb, don't close your own prs ;)

@parikhshiv
Copy link
Contributor Author

Sure @narenranjit (and noted in terms of future PRs) -

Basic idea of the centered axis is to vertically center the y axis around data points.

An updated example of use of the centered axis can be seen here - http://jsfiddle.net/a5L7x0c5/3/ (also demonstrates new placement of zeroAnchor and smartAxis flags).

All changes are backwards compatible (tested), but we may want to update docs re: formatting of yAxis options with the new 'scaling' attribute.

@mmrj
Copy link
Contributor

mmrj commented Jan 11, 2017

@parikhshiv thanks! on my list to review, but behind 2 x epicenterjs releases ... will let you know as i have questions.

@narenranjit
Copy link
Contributor

narenranjit commented Jan 11, 2017

Moving those options to 'scaling' is a start, but aren't some of those options contradictory? i.e scaling: { smartAxis: true, centeredAxis: true } will never makes sense/work; it'll always be one or the other.

Rather than just updating the docs saying "don't use both", a good API should prevent them from doing both if at all possible. One way to do this is to change the above line to read scaling{ type<String>:"smart or centered or ...", options: <any further config options required by the scaling algorithm, like the zeroanchor> } ; this'll make it impossible to turn on both smart and centered at the same time.

Another advantage of doing it this way is it reduces the testing surface area. i.e, rather than write a different test case for "smart:true, center:true" "smart: true, center: false" and all the other permutations, you just write 1 per config options.

Let me know if this makes sense, or if i misunderstood how those options work (entirely possible).

@parikhshiv
Copy link
Contributor Author

No, this makes sense to me @narenranjit - I can update the structure of the scaling option according to what you've specified above.

This may take a few days (just a heads up in terms of updating docs, @mmrj ).

@parikhshiv parikhshiv mentioned this pull request Jan 12, 2017
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