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

Custom View Items #5

Closed
wants to merge 3 commits into from
Closed

Custom View Items #5

wants to merge 3 commits into from

Conversation

felipowsky
Copy link
Contributor

The main goal when I started to change PickerView control was to implement custom view items for a project. I ended up implementing a lot more than that.
I don't know whether you'll agree with all the changes that I did but I'm sending this pull request because I really like the result.

Summary of what I've done:

  • Added support for custom view items
  • Added compatibility with Objective-C
  • Added optional for function delegates that aren't mandatory
  • For every function delegate that has row parameter added an index parameter. This parameter is pretty handy when dealing with an infinite PickerView. row is the indexPath.row from the internal tableView while index is the index considering indexPath.row % items.count
  • Added a new function delegate to know when a row was tapped: pickerView(PickerView, didTapRow row: Int, index: Int)
  • Changed the name of some function delegates to conform with Apple's UIPickerViewDelegate. The names of function delegates were working fine but I prefer to work with their names as close as Apple's.

I know, there's a lot of changes. I should have separated each implementation but I changed the control directly while I was developing the project that I mentioned before. Sorry for that.

I added the custom view option in the Example project for better reference and testing.

Resolves #4

…ods, added support for custom views, added select row method and added current selected index property
…ll delegates that use row, fixed first row to center
@filipealva
Copy link
Owner

Hello @felipowsky. First of all: Thank you so much for contributing!

I really appreciated your contributions.

I've related this pull request to issue #4 which is where the custom view as an item was requested and I will return tomorrow to finish the review.

tks

@felipowsky
Copy link
Contributor Author

No problem.
Let me know what you think and I can help with any changes.

I just noticed I didn't select the correct branch.
If you want we can discuss the changes here and I create a new pull request with a separate branch for merging.

@filipealva
Copy link
Owner

I reviewed it yesterday, but I didn't had time to bring some "philosophical" questions that I would like to discuss with you, sorry about that.

Now I'm heading to your lands (SC), I will stay in Camboriu until January 4, so I think we can close this pull request and discuss the changes in the new one, but if you want to discuss here no problem.

What do you prefer?

Sorry about the delay, my intention was to had it merged before the year ends but this year ending is crazily busy for me :(

@filipealva
Copy link
Owner

Hello! I'm back, happy 2016 bro!

So, I think the only doubt I had was:

We have ScrollingStyle and SelectionStyle to clearly determine how PickerView style will behave.

What do you think about implement the ItemsType enum as nested type (and a property) of PickerView such as the other enums which determine style behavior?

I got that you determine if it will be a label or a custom view verifying which delegate method the developer has implemented, but it looks a little “abstract”. If we had it as a property of PickerView we could set a value as default (probably .Label) and use it to verify what kind of behavior the developer wants with this property instead of verifying which delegate methods the developer has implemented. See the following lines:

let view = delegate?.pickerView?(self, viewForRow: indexPath.row, index: indexForRow(indexPath.row), highlited: indexPath.row == indexOfSelectedRow, reusingView: pickerViewCell.customView)

if (view != nil) {
    // Implement customView
} else {
    // Implement label
}

Would be something like that:

switch itemsType {
case .Label: 
   // Implement label
case .CustomView:
   // Implement custom view
}

In that way the behavior looks more clearly to me, but I really want to hear what do you think about it.

@felipowsky
Copy link
Contributor Author

Happy 2016! :)

Hum, I see. It's ok for me.

I think this is more like a personal choice.
My implementation was based on Apple's UIPickerView which considers whether custom view delegate is implemented or not.
Actually, Apple's UIPickerView implementation is more strict because once the custom view delegate is implemented the control ignores the default label completely even if the custom view delegate returns nil.

My intention was to give the developer the impression of using Apple's UIPickverView control and also the possibility to mix custom view and default labels. Honestly I don't know if they want that and I haven't tested the "mixing" feature to see if it really works.

Well, I'm fine if you want to change it.

@filipealva
Copy link
Owner

Sorry about the delay.

I got your point, but I will make the changes to make sense with the other features.

You said before that you would create a new pull request. Should I wait for it to make the changes?

tks

@felipowsky
Copy link
Contributor Author

Great.

I've created a new branch with my changes.
New pull request: #6

@felipowsky felipowsky closed this Jan 12, 2016
@felipowsky felipowsky mentioned this pull request Jan 12, 2016
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.

Feature request - add support for cell contents to be a view
2 participants