search

ivnsch / SwiftCharts

Easy to use and highly customizable charts library for iOS
Apache License 2.0
2.53k stars 410 forks source link

#304: Implemented gradient fill for curved areas #305

Closed dehlen closed 7 years ago

dehlen commented 7 years ago
ivnsch commented 7 years ago

Looks good, thanks! I added a few comments.

Also: If you like, you can add a screenshot of the gradients to the readme. It would be nice to show this! In case you do, you can replace the "target point animation" screenshot.

dehlen commented 7 years ago

Hey, thanks for coming back at me. I just wanted to let you know that I saw your comments and I will update this PR as soon as possible. I might get some freetime on Sunday, at the latest Monday.

dehlen commented 7 years ago

Hey, I just pushed a new commit to the feature branch updating the code accordingly to the comments you made :)

ivnsch commented 7 years ago

Thanks a lot!

dehlen commented 7 years ago

Hey @i-schuetz I just saw that you closed the PR but not merged it. Just wanted to ask if this was intentional or not. No bad feelings here if the PR should be updated/improved ;) Just wanted to make sure you did not accidentally close this issue without merging.

Thanks, David

ivnsch commented 7 years ago

My bad!!! sorry.

ArturoLee commented 7 years ago

Had to delete the changes to ChartPointsAreaLater and ChartAreasView to get it to work again.

ivnsch commented 6 years ago

@ArturoLee I'll look into this tomorrow!

dehlen commented 6 years ago

I think this could be fixed by setting addContainerPoints to false when using a line graph with a StraightLinePathGenerator. But this is just a guess I did not have any time to look into this more deeply by now. I could also have a look in the next couple of days. lineLayer is a parameter here because the gradient ist generated with the same pathGenerator one is using to draw the line because it wouldn't make sense to draw a line with a StraighLinePathGenerator but drawing the gradient under it with a CatmullPathGenerator for example.

Cheers, David

ivnsch commented 6 years ago

@ArturoLee @dehlen

I changed the implementation to accept a path generator instead of a line layer. The dependency to line layer is circumstantial and shouldn't be enforced in this way (this totally escaped my review) - it should not be required to have a line layer to create an area chart!

Further I'm not sure if it makes sense to keep addContainerPoints as a parameter. Are there charts which don't need this?

Anyway, the container points should be added to the points array passed to the path generator instead of being appended to the core graphics path after the bezier path was already created. @dehlen can you do this?

Sloppy review, sorry for that. I'll also do a release once this is fixed so you don't have to use master.

ivnsch commented 6 years ago

A bit late, but the problem with the straight paths should be fixed now (in master)!