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

Added gradient line drawing to LineChartRenderer #3142

Closed
wants to merge 1 commit into from

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Dec 29, 2017

Reimplemented #649 for modern Swift and Charts

@natacodes
Copy link

natacodes commented Dec 30, 2017

Excited to see this re-implemented for a recent version of Charts. Thank you, @jjatie 👍🏻
Could you provide an example usage? I pulled your branch, added the following code but I don't get any gradient:

        set1.isDrawLineWithGradientEnabled = true
        set1.gradientPositions = [40.0, 120.0]

EDIT: I was missing set1.setColors([color1, color2, color3], alpha: 1.0)

I still can't get a nice gradient line. Let's say I want everything below 50 to be red, between 50 and 100 to be yellow and 100+ to be green. What values should I put it setColors and gradientPositions?

I tried this:

        // values: 50, 300, 50
        set1.mode = .linear
        set1.isDrawLineWithGradientEnabled = true
        set1.setColors([.red, .green, .yellow], alpha: 1.0)
        set1.gradientPositions = [80, 130, 200]

Result:
img_7a33b4493ccf-1

Also it might be a good idea to expose gradient colours as an open variable.

A few things I noticed:

  • It's only implemented for cubicBezier and linear, missing cubicHorizontal and stepped
  • isDrawLineWithGradientEnabled is set as { get set } instead of following the convention used in this library which is having drawLineWithGradientEnabled -> { get set } and isRawLineWithGradientEnabled -> { get }
  • With certain values in data set I'm getting Thread 1: Fatal error: Can't form Range with upperBound < lowerBound


// create a new path
let to = Int(ceil(CGFloat(to - from) * phaseX + CGFloat(from)))
for i in (from + 1)..<to
Copy link

@natacodes natacodes Dec 30, 2017

Choose a reason for hiding this comment

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

_xBounds.max and _xBounds.min can be zeros when there's only one data entry and this for is crashing the app with Fatal error: Can't form Range with upperBound < lowerBound

Choose a reason for hiding this comment

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

It needs if dataSet.entryCount > 1 somewhere.

@liuxuan30
Copy link
Member

@natacodes thanks! good to have you help reviewing and providing feedback.
We plan this for 4.0 feature.

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 2, 2018

@natacodes Thanks for the feedback. To be honest, I blindly reimplemented the solution from #649. I just wanted to get the ball rolling again on what I thought was a good feature. I would like to have an API that is unambiguous as to what values to provide as you mentioned.

I will hopefully have time in the next week or so to more closely evaluate and test the solution. In the meantime, I encourage you to PR any suggestions you have for the implementation into this branch.

@natacodes
Copy link

I encourage you to PR any suggestions you have for the implementation into this branch.

@jjatie I tried, but it's a little bit too complex for me at this time.

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 24, 2018

@natacodes No problem. This will take a little time for us to get around to, but I hope you are able to continue provide feedback when we do. Thank you.

@liuxuan30
Copy link
Member

I actually wanted to try this out in my new app after we take care of others.

afineon added a commit to afineon/Charts that referenced this pull request Mar 1, 2018

for _ in dataSet.colors
{
cColor = dataSet.color(atIndex: 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jjatie just want to check that this line is correct. It iterates over dataSet.colors, but always takes color at index 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

    for i in dataSet.colors.indices
    {
        cColor = dataSet.color(atIndex: i)

pmairoldi added a commit that referenced this pull request Jun 10, 2018
Added gradient line drawing to LineChartRenderer. based on PR #3142
@larryonoff
Copy link
Contributor

I think this PR can be closed now, since PR #3415 is based on this one and it was just merged into branch 4.0.0.

@pmairoldi pmairoldi closed this Jun 11, 2018
@Ferszt
Copy link

Ferszt commented Aug 6, 2018

Hey guys, can this functionality be used in production now?

@larryonoff
Copy link
Contributor

@Ferszt please use branch #4.0.0. The functionality already there.

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.

6 participants