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

Fix #1685 - add adjustable padding between each bar in discreteBar #2136

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

avdeveloper
Copy link
Contributor

This minimal update will allow dev's to make the bars in discreteBar charts to be narrower

@@ -216,6 +217,7 @@ nv.models.discreteBar = function() {

chart._options = Object.create({}, {
// simple options, just get/set the necessary values
barWidth:{get: function(){return barWidth;}, set: function(_){barWidth=_;}},
Copy link
Contributor

Choose a reason for hiding this comment

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

barWidth only makes sense to be between 0 and 1 here right? So it should probably validate that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liquidpele if it doesn't validate, should I just return the default value or do a console.error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would nv.log it (wrapper around console.log we use) and ignore the passed value.

@liquidpele
Copy link
Contributor

for new options, please also add a description/example in the json object in the bottom of documentation.html

@albert-tse
Copy link

Ok let me do that

@liquidpele
Copy link
Contributor

Any update?

@avdeveloper
Copy link
Contributor Author

oh yeah! let me update that on sunday XD

@avdeveloper
Copy link
Contributor Author

@liquidpele welp, I committed late, but at least it's on a Sunday XD

@compwron
Copy link

I wish this were merged, it would be great for my use case.

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

Successfully merging this pull request may close these issues.

4 participants