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 gradient bars for BarChart [master branch] #4411

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

Conversation

larryonoff
Copy link
Contributor

This PR adds functionality to draw gradient bars with BarChartDataSet based on master branch

if let set = chartView.data?.dataSets.first as? BarChartDataSet {
set1 = set
set1.replaceEntries(yVals)
setup(set)
Copy link
Member

Choose a reason for hiding this comment

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

let's rename to a more meaningful name to imply it's about setup gradient

@@ -174,6 +175,11 @@ class LineChart1ViewController: DemoBaseViewController {
set.mode = (set.mode == .cubicBezier) ? .horizontalBezier : .cubicBezier
}
chartView.setNeedsDisplay()
case .toggleGradient:
Copy link
Member

Choose a reason for hiding this comment

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

is it possible for you to add the objc portion? (feel free to say no)

if dataSet.drawBarGradientEnabled {
drawGradientBars(context: context, dataSet: dataSet, buffer: buffer, matrix: valueToPixelMatrix)
} else {
drawDefaultBars(context: context, dataSet: dataSet, dateSetIndex: index, buffer: buffer)
Copy link
Member

Choose a reason for hiding this comment

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

maybe drawNormalBars or standard bars, etc? not sure which is better. @jjatie idea?

context.setLineWidth(dataSet.barBorderWidth)
context.stroke(barRect)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

in gradient bars, accessibilityOrderedElements is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liuxuan30 pushed commit. Hope that I did it correct.

@@ -124,6 +140,26 @@ public class NSUIDisplayLink
}
}

extension NSUIColor
{
var nsuirgba: (red: CGFloat, green: CGFloat, blue: CGFloat, alpha: CGFloat)? {
Copy link
Member

Choose a reason for hiding this comment

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

though the imp is different, here are two var nsuirgba in one extension? typo? or two vars indeed?

Copy link
Member

Choose a reason for hiding this comment

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

@larryonoff can you get the notification? I basically finished the review, jsut some minor issues to address.

@@ -14,6 +14,22 @@ public typealias NSUIScrollView = UIScrollView
public typealias NSUIScreen = UIScreen
public typealias NSUIDisplayLink = CADisplayLink

extension NSUIColor
{
var nsuirgba: (red: CGFloat, green: CGFloat, blue: CGFloat, alpha: CGFloat)? {
Copy link
Member

Choose a reason for hiding this comment

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

move { to a new line

@liuxuan30
Copy link
Member

@jjatie @danielgindi please also take a look. Generally I'm accepting except for some minor issues.

@prats-iosdeveloper
Copy link

Hi i want to implement gradient bars with round corners when to expect this merge?

@larryonoff
Copy link
Contributor Author

larryonoff commented Jul 22, 2020

@liuxuan30 @danielgindi Hi. Sorry for the delay in responding. Really busy these weeks. I won't be able to check this PR for some time. But I think It can be done without my help easily.

@liuxuan30
Copy link
Member

liuxuan30 commented Jul 27, 2020

@larryonoff I could fix most of them, but two left need your input,

1st is about accessibilityOrderedElements in the new code, I didn't see it. Did you just forget or because there are issues?

2st is about the var nsuirgba, I need your input which is new, because you added two same var with different implementation. And I can't tell which is the right one.

rename `setup(_ dataSet: BarChartDataSet)` to `setupBarGradient(_ dataSet: BarChartDataSet)`
rename  `drawDefaultBars` to `drawNormalBars`
@liuxuan30 liuxuan30 changed the base branch from master to GradientBars July 27, 2020 02:15
@larryonoff
Copy link
Contributor Author

@liuxuan30

1st is about accessibilityOrderedElements in the new code, I didn't see it. Did you just forget or because there are issues?

just pushed commit

2st is about the var nsuirgba, I need your input which is new, because you added two same var with different implementation. And I can't tell which is the right one.

nsuirgba implementation differs for macOS and iOS.

  • iOS uses UIColor.getRed, which returns success / not and only available for iOS .
  • macOS uses NSColor.getRed, which doesn't return any values (I'm not sure, but it may crash if input parameters doesn't fit RGB model).

@liuxuan30
Copy link
Member

@larryonoff I saw nsuirgba is ported from 4.0 branch.

I was evaluating which branch is better to merge, merging into master will have more conflicts while merging 4.0 branch, so I was kind of unsure.

anyway, I could take on from here. Thanks!

@liuxuan30
Copy link
Member

liuxuan30 commented Jul 28, 2020

I think the best way is to merge 4.0 branch first, and try to merge this.

I'm actively working on the 4.0 merge right now, and it could take 1-2 weeks to resolve the conflicts.

@danielgindi @jjatie

fatbobman added a commit to fatbobman/Charts that referenced this pull request Oct 15, 2020
@StackHelp
Copy link

@liuxuan30 Can we have a merged file in next few weeks?

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 6, 2020

I think so, @larryonoff so we have merged the 4.0 branch into master, now this is the next we want to merge. I'm switching to master base

@liuxuan30 liuxuan30 changed the base branch from GradientBars to master November 6, 2020 07:36
@liuxuan30
Copy link
Member

@larryonoff any chance you could fix the conflicts?

@larryonoff
Copy link
Contributor Author

@liuxuan30 hey! sorry, but not in a week. very busy.

@StackHelp
Copy link

@larryonoff @liuxuan30 Any chance to update in this week?

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 14, 2020

Sorry I‘m super busy recently as well. I hope someone could help on this PR, basically solve the conflict with master.

@4np
Copy link
Contributor

4np commented Dec 15, 2020

This would be easy to accomplish by providing your custom renderer, if #4297 was merged.

@4np 4np mentioned this pull request Jan 29, 2021
@glennposadas
Copy link

glennposadas commented Feb 16, 2021

Hi everyone! Sorry for hijacking this PR. But I';ve been reading the commits, and I still have no idea how to apply gradient colors to my HorizontalBarChartView. I don't see any property too in BarChartDataSet that is related to gradient. I'm using 4.0.0. Any idea? Thanks!

EDIT: I suppose this hasn't been merged to the Master.

@dbrisinda
Copy link

May 2023 and still waiting on this...

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.

8 participants