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

V2.0 Dev Feature Horizontal Bar #2434

Closed
wants to merge 3 commits into from
Closed

V2.0 Dev Feature Horizontal Bar #2434

wants to merge 3 commits into from

Conversation

potatopeelings
Copy link
Contributor

As mentioned in #73.

I cleaned up the code a bit (verified that it still works). Cheers!

@potatopeelings
Copy link
Contributor Author

I think I've fessed up a couple of variables. Will update in a bit.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 35754fd on potatopeelings:v2.0-dev-feature-horizontal-bar into * on chartjs:v2.0-dev*.

@coveralls
Copy link

coveralls commented May 2, 2016

Coverage Status

Changes Unknown when pulling be62657 on potatopeelings:v2.0-dev-feature-horizontal-bar into * on chartjs:v2.0-dev*.

@tannerlinsley
Copy link
Contributor

This looks pretty dang good man! @etimberg What do you think?

@derekperkins
Copy link
Contributor

Do negative values work?
On May 2, 2016 12:36 PM, "Tanner Linsley" notifications@github.com wrote:

This looks pretty dang good man! @etimberg https://github.com/etimberg
What do you think?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#2434 (comment)

@etimberg
Copy link
Member

etimberg commented May 2, 2016

Looks good. Would love to get some tests on it? if @potatopeelings can't, could you do that @tannerlinsley ?

@potatopeelings
Copy link
Contributor Author

@derekperkins - yep, negative values work (not that I did anything special for it :-)). Fiddle from an earlier version - http://jsfiddle.net/0y8mnma1/.

@etimberg - I haven't written tests for chart libraries before unfortunately. If there's anything else I can do (example files?), am glad to give it a shot. Cheers!

@tannerlinsley
Copy link
Contributor

I might have time later to put up some basic tests. I'm still not familiar with the new testing strategy, but there's no better time to check it out than now. We'll see how my day goes.

@etimberg
Copy link
Member

etimberg commented May 2, 2016

Ok, I like the fiddle. I am +1 to merge this.

If we do merge, could I propose the following:

  1. Merge
  2. Add sample file
  3. Add test coverage
  4. 2.1 release?

@tannerlinsley
Copy link
Contributor

Yep, I think this would round out the 2.1 release.

@tannerlinsley
Copy link
Contributor

I was wondering though, do you think instead of creating a new controller, we use the existing bar controller and just make it compatible for the vertical position? I'm not sure. Just came to my mind.

@etimberg
Copy link
Member

etimberg commented May 2, 2016

That would work, but most functions would have to change. An alternative is to derive obw from the other to keep duplication to a minimum

@etimberg
Copy link
Member

etimberg commented May 2, 2016

It also needs it's own default config which is better done with a new type

@etimberg
Copy link
Member

etimberg commented May 2, 2016

Just realized this is going into v2.0-dev and not master

@potatopeelings
Copy link
Contributor Author

Should I have branched from master instead? Sorry! Let me know if that's the case and I can raise a new PR.

@etimberg
Copy link
Member

etimberg commented May 2, 2016

Master would be preferred

@potatopeelings
Copy link
Contributor Author

Sure. Will get it done after a few hours.

@etimberg
Copy link
Member

etimberg commented May 2, 2016

thanks @potatopeelings

@swbdev
Copy link

swbdev commented May 3, 2016

Just checked the logarithmic scale function in the fiddle. It seems to have some problems. Btw. is it possible to change the labels of the axes to normal integer values when logarithmic scale is applied? (check the screens)

bar
horizontalbar

Edit:
My fault. Log scale seems to work great:
download

@etimberg
Copy link
Member

etimberg commented May 3, 2016

@swbdev you need to add the following to the logarithmic scale config. The default position for the log scale is on the left

position: 'bottom'

@potatopeelings
Copy link
Contributor Author

@etimberg - created a new PR for master (#2448). Cheers!

@etimberg
Copy link
Member

etimberg commented May 3, 2016

Great! Closing this one

@etimberg etimberg closed this May 3, 2016
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.

6 participants