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 area diagram functionality #42

Closed
wants to merge 13 commits into from

Conversation

hackingotter
Copy link

Hello

I like JBchartView, but I missed the possibility to fill the area under the lines to create a area diagram so I added it. I also added the area diagram to the sample.

I tried to follow the code style of the rest of the porject.

screenshot 2014 04 22 16 58 09

@simonbromberg
Copy link

This would be a great addition.

@hackingotter
Copy link
Author

Now it works with the smoothed curves of Version 2.4.0

ios simulator bildschirmfoto 29 04 2014 17 31 13

@simonbromberg
Copy link

Wobble issue for linear lines still seems to affect the bottom of a section
ios simulator screen shot apr 29 2014 1 10 30 pm

@hackingotter
Copy link
Author

I forgot to change the slope calculation for the bottom line. Now it works.
ios simulator bildschirmfoto 29 04 2014 19 23 38

@hackingotter
Copy link
Author

@terryworona Is there any particular reason, why this pull reequest hasn't been merged so far? I'm just wondering why it didn't happen until now.

@terryworona
Copy link
Collaborator

@hackingotter been meaning to look it over. This weekend! Promise!

@hackingotter
Copy link
Author

great. Thank you

@@ -14,8 +14,8 @@

@interface JBBarChartView : JBChartView

@property (nonatomic, weak) id<JBBarChartViewDelegate> delegate;
@property (nonatomic, weak) id<JBBarChartViewDataSource> dataSource;
@property (nonatomic, weak) IBOutlet id<JBBarChartViewDelegate> delegate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these IBOutlets (forgive my naiveness, I never use IB); can the chart not be used with IB without these declared as outlets?

Copy link
Author

Choose a reason for hiding this comment

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

the IBOutlets make it possible to set the delegate and the dataSource directly in IB, like it's possible for standard UI elements like UITextField. It's just for convinience, the charts can be used without the IBOutlets, too.
screenshot 2014-05-05 10 33 56

@terryworona
Copy link
Collaborator

Looked over the request. Generally looks good, but not quite ready for prime-time yet.

There's a bunch of duplicate code that I think can be condensed. As well, I'd like to simply the interface for filling in lines.

What do you think?

@hackingotter
Copy link
Author

I will try to remove the duplicate code. This will also improve the intergrationof future improvements.

A simpler interface also would be nice. I'll use a delegate methode for the filling and for the filling color I will use the line color and lower the alpha so you still can seperate between line and the filling.

About the stacked/cumulative lines I think that this feature should be left as it is, as I mentioned above.
It also solves the problem with the filling if two lines intersect each other.

@hackingotter
Copy link
Author

Ok, I changed a lot and condensed most of the duplicates but at one point there still is one (marked with FIXME) I don't know how to remove.

For details loock at the comments I made.

@skywinder
Copy link
Contributor

Yay! It's very cool feature! Thanks!

@hackingotter
Copy link
Author

@terryworona Could you please review the changes I made?

@terryworona
Copy link
Collaborator

Hey @hackingotter, thanks so much for the contribution and continued work on getting this polished.

After a bit of discussion, we have decided to not pull in this particular feature, for a few reasons:

  1. It's a rather large chunk of code -- not saying this is a bad thing, it's just that we prefer smaller, more manageable contributions (we are quite busy with our own work, and currently don't have the bandwidth to support large (untested) feature additions).
  2. We have been working on our own implementation of area graphs; it will be a new subclass of JBChartView and contain a similar interface to JBLineChartView.
  3. After running the branch through it's paces, it does still have some issues related to padding. Rather than make you fix these, re-test and potentially not pull it in, we would rather be honest.

Your contributions are not at all lost and will be used going forward with our area graphs. You will be given contributor credit via the PR when the time comes.

Thanks again!

@skywinder
Copy link
Contributor

But a little improvement with IBOutlet is good feature to support storyboards:
+@property (nonatomic, weak) IBOutlet id<JBBarChartViewDelegate> delegate;

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.

None yet

4 participants