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

Add support for floating bar chart ([start, end]) #6056

Merged
merged 4 commits into from
May 21, 2019

Conversation

gwyneblaidd
Copy link
Contributor

@gwyneblaidd gwyneblaidd commented Feb 8, 2019

Rebased float-bar support

Original PR: #5262

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

It looks like this is very close. There are two failing tests still

src/core/core.scale.js Outdated Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
@simonbrunel simonbrunel changed the title Rebased float-bar support. Flhttps://github.com/chartjs/Chart.js/pull/5262 Add support for floating bar chart ([start, end]) Feb 9, 2019
src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Things to consider, I'm not saying they should be done though 😄

src/controllers/controller.bar.js Outdated Show resolved Hide resolved
src/core/core.scale.js Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/controllers/controller.bar.js Outdated Show resolved Hide resolved
benmccann
benmccann previously approved these changes Feb 18, 2019
src/controllers/controller.bar.js Outdated Show resolved Hide resolved
benmccann
benmccann previously approved these changes Feb 18, 2019
@benmccann
Copy link
Contributor

@gwyneblaidd #6077 which implements the option not to skip borders has been merged now, so you shouldn't need any changes to element.rectangle.js anymore. Can you rebase the PR? Hopefully the other reviewers can take a look at this PR and we can merge it after that. It's very close. Thanks for sticking with it

I normally don't recommend squashing commits, but I find that if you have many commits then squashing them first makes rebasing easier, so you may want to consider in this case (git rebase HEAD~16, replace pick with s for all but the first commit, comment out all but the first commit message, git push -f)

benmccann
benmccann previously approved these changes Mar 3, 2019
@benmccann benmccann mentioned this pull request Mar 8, 2019
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Show resolved Hide resolved
src/controllers/controller.bar.js Show resolved Hide resolved
_autoSkipp rolled back

minbarlength. float-bar

start fix

start fix

fix

fix

fixes

fix

fix

fix

fixes

unittests

comment removed

reverse of borderSkipped
benmccann
benmccann previously approved these changes Mar 30, 2019
etimberg
etimberg previously approved these changes Mar 30, 2019
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Also, shouldn't we add docs in this PR?

@xaviRodri
Copy link

xaviRodri commented Apr 4, 2019

Good functionality!
Only one question: this [start, end] will only work for the y-axis, or will we be able to do the same in x-axis too?
Something like: {x: [start, end], y: value}
And a result like this:
image

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.