FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Draw the rightmost grid line when offsetGridLines is true by nagix · Pull Request #6326 · chartjs/Chart.js · GitHub

/ Chart.js Public

Draw the rightmost grid line when offsetGridLines is true - #6326

Merged
etimberg merged 5 commits into
chartjs:masterfrom
nagix:issue-6326
Jul 18, 2019
ConversationCommits (5)ChecksFiles changed
Merged

Draw the rightmost grid line when offsetGridLines is true#6326
etimberg merged 5 commits into
chartjs:masterfrom
nagix:issue-6326

Conversation

nagix commented Jun 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

With this PR, the rightmost grid line in bar charts, where offset and gridLines.offsetGridLines are true by default, will be drawn. It also fixes the following minor issue.

  • If gridLines.color is an array but some element is undefined, that line won't be drawn. It should rather be drawn in the default color.

I'm aware of #5480 trying to solve this by a new plugin, but there have been many changes in the core scale code since it was opened, and a lot of effort would be needed to rebase it. This PR doesn't prevent to introduce a separate grid line plugin, but proposes a simpler approach to address the issue based on the latest code base.

Master: https://jsfiddle.net/nagix/9ak5L48q/

This PR: https://jsfiddle.net/nagix/dLv84ujs/

Fixes #3804

benmccann commented Jun 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

I think #5480 is trying to do too much in a single PR. It'd be much easier to review if we added the new border functionality and separated the gridline functionality as a plugin in separate PRs, so I'm okay moving forward with this PR separately.

etimberg requested review from kurkle and simonbrunel June 9, 2019 00:48
etimberg previously approved these changes Jun 9, 2019
View reviewed changes
kurkle previously approved these changes Jun 12, 2019
View reviewed changes
Comment thread src/core/core.scale.js Outdated
benmccann reviewed Jun 12, 2019
View reviewed changes
Comment thread src/core/core.scale.js Outdated
nagix dismissed stale reviews from kurkle and etimberg via 62442ff June 17, 2019 10:01
kurkle previously approved these changes Jun 23, 2019
View reviewed changes
benmccann reviewed Jun 23, 2019
View reviewed changes
Comment thread src/core/core.scale.js
Comment thread src/core/core.scale.js Outdated
Comment thread src/core/core.scale.js Outdated
benmccann previously approved these changes Jun 24, 2019
View reviewed changes

benmccann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm.

I might consider splitting _computeItemsToDraw into _computeGridLineItems and _computeLabelItems as it's getting pretty big and it looks like there's essentially no shared code between them. It'd also mean you wouldn't need the extra variable anymore

nagix commented Jun 26, 2019

Copy link
Copy Markdown
Contributor Author

@benmccann I like that idea. It will give some performance improvement if either grid lines or labels are hidden.

benmccann previously approved these changes Jun 26, 2019
View reviewed changes

benmccann commented Jul 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@nagix lgtm, but this PR will need to be rebased

etimberg previously approved these changes Jul 16, 2019
View reviewed changes
nagix dismissed stale reviews from etimberg and benmccann via a351d7a July 17, 2019 13:06

nagix commented Jul 17, 2019

Copy link
Copy Markdown
Contributor Author

Rebased

snyk-bot mentioned this pull request Apr 1, 2020
Open
This was referenced Apr 14, 2020
Open
Open
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Draw the rightmost grid line when offsetGridLines is true (chartjs#6326)
e028b02 
* Draw the rightmost grid line when offsetGridLines is true

* Refactor based on feedback

* Replace helpers.each with for loop

* Minor refactoring

* Refactor _computeItemsToDraw
This was referenced Jun 1, 2021
Closed
Merged
Closed
Open
Closed
Open
Open
This was referenced Jul 5, 2021
Closed
Open
This was referenced Jul 27, 2021
Closed
Merged
Merged
This was referenced Oct 14, 2021
Merged
Closed
jw mentioned this pull request Jan 17, 2022
Open
1 task
This was referenced Jan 21, 2022
Merged
Closed
This was referenced Feb 4, 2022
Closed
Closed
This was referenced Mar 7, 2022
Open
Open
Open
mend-for-github-com Bot mentioned this pull request Aug 13, 2022
Merged
1 task
This was referenced Nov 29, 2022
Closed
Closed
mend-for-github-com Bot mentioned this pull request Jan 26, 2023
Closed
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

etimberg etimberg approved these changes
kurkle kurkle left review comments
simonbrunel Awaiting requested review from simonbrunel
+1 more reviewer
benmccann benmccann approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: enhancement

Projects

None yet

Milestone

Version 2.9

Development

Successfully merging this pull request may close these issues.

[Possible Bug] Bar Chart doesn't display right border

4 participants

Back | FazBrowse Home | New Git URL