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.

Sign up for GitHub

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

Jump to bottom

Proposal: Add support for multiple cells types #390

Merged
merged 8 commits into from Oct 20, 2019
Merged

Proposal: Add support for multiple cells types #390

merged 8 commits into from Oct 20, 2019

Conversation

yosshi4486
Copy link
Contributor

@yosshi4486 yosshi4486 commented Oct 2, 2019 •
edited

Proposal

Add support for multiple cells types

@rechsteiner
Copy link
Owner

Hi @syatyo! Thank you so much for your PR. I have a few comments on your proposal.

  • I don't think we should add cellReuseIdentifier to the PagingItem protocol. Registering multiple cells is quite an edge-case and I don't think we should force everyone to implement it. There is always a one-to-one mapping between items and cells, so I think it's safe to use String(describing: self) of the item type as the cell identifier.

  • Instead of changing menuItemSources to an array I was thinking of getting rid of that property altogether. I was thinking of making the API similar to the UICollectionView cell registration, like this:

pagingViewController.register(LoadingCell.self, for: LoadingItem.self)
pagingViewController.register(nib, for: LoadingItem.self)

By default, PagingTitleCell will be registered.

What do you think about that?

@yosshi4486
Copy link
Contributor Author

yosshi4486 commented Oct 19, 2019 •
edited

Thank you for your comments @rechsteiner !

  • It's good idea using String(describing: self). I had been wondering about using proper cell-reuseIdentifier in making a collection view cell. Your approach seems better 👏

  • New APIs are so simple and good!

I'll try them!

@yosshi4486
Copy link
Contributor Author

Hi @rechsteiner !

I have just committed them following your comments. Please review them🙇

yosshi4486
yosshi4486 commented Oct 19, 2019
View reviewed changes
Copy link
Contributor Author

@yosshi4486 yosshi4486 left a comment •
edited

Choose a reason for hiding this comment

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

Important changes. I've already checked that all tests are green

Parchment/Classes/PagingOptions.swift

/// The class type for the menu item. Override this if you want
/// your own custom menu items. _Default: PagingTitleCell.self_
public var menuItemSource: PagingMenuItemSource
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get rid of old API.

Parchment/Classes/PagingController.swift
let cell = collectionView.dequeueReusableCell(
withReuseIdentifier: PagingController.CellIdentifier,
withReuseIdentifier: String(describing: type(of: pagingItem)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use Type name of pagingItem instance to decide proper cell identifier.

Parchment/Classes/PagingViewController.swift Outdated
/// Register cell class for paging cell
/// - Parameter cellClass: paging cell's class
/// - Parameter pagingItemType: paging item type for specifying cell identifier
public func register(_ cellClass: AnyClass?, forCellWithPagingItemType pagingItemType: PagingItem.Type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new API to PagingViewController as register(_ cellClass: AnyClass?, forCellWithPagingItemType pagingItemType: PagingItem.Type). I tried to use similar argument name to UIKit.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you rename forCellWithPagingItemType to just for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completed to rename them!

Copy link
Owner

Choose a reason for hiding this comment

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

Great! One more thing, any reason AnyClass/UINib is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only for consistency to UIKit API.

@rechsteiner
Copy link
Owner

Awesome, thank you so much for adding this 🙌

@rechsteiner rechsteiner merged commit 841adac into rechsteiner:2.0 Oct 20, 2019
@rechsteiner rechsteiner mentioned this pull request Oct 20, 2019
Closed
5 tasks
@yosshi4486 yosshi4486 deleted the feature/multiple-cells-types branch October 20, 2019 20:26
rechsteiner pushed a commit that referenced this pull request Nov 16, 2019
@yosshi4486 @rechsteiner
Add support for multiple cell types (#390)
f6ded96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rechsteiner rechsteiner rechsteiner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yosshi4486 @rechsteiner