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

Minor refactor for BarLineChartViewBase #268

Merged
merged 1 commit into from
Aug 11, 2015

Conversation

liuxuan30
Copy link
Member

  1. change all gestures and their handlers to internal
  2. use import UIKit instead of UIKit.UIGestureRecognizer

@pmairoldi
Copy link
Collaborator

What are the benefits of making them internal?

@liuxuan30
Copy link
Member Author

then you can override it with your desired actions.

@pmairoldi
Copy link
Collaborator

The reason I am asking is that those methods are functions specific to those gesture recognizers. Its not as simple as overriding the the subclass. I'd see a case for either making the just the variables internal or just the functions internal. I think both will lead to confusion when users override stuff.

@liuxuan30
Copy link
Member Author

If both internals makes confusion, I would like to make the functions internal.

But sometimes, we may want to replace the gestures to be our sub classes. I used to write a multiple tap gesture recognizer, and when I tried to replace it, I cannot since it's private. This is the reason why I make both internal.

It is not simple to override, but they are for advanced users :) Users are responsible for what they are doing when subclassing.

@pmairoldi
Copy link
Collaborator

I think out of the two making the variables internal is better. The object defines what selector will get called. If you want to change the tap recognized then just replace whole object I your subclass and implement the selector. it also adds more flexibility like changing numberOfTaps etc.

change all gestures to internal
Use import UIKit instead of UIKit.UIGestureRecognizer
@liuxuan30
Copy link
Member Author

You seems right. I don't come up this way. PR updated.

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.

None yet

3 participants