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

Add support for registering decoration view classes rather than nibs #1

Closed

Conversation

swoolcock
Copy link

UICollectionViewLayout usually supports registering decoration views as both nibs and classes.
This change is to allow the SpreadsheetLayout subclass to also provide that functionality.

Copy link
Owner

@stuffrabbit stuffrabbit left a comment

Choose a reason for hiding this comment

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

Hi thank you for the contribution.
I would suggest to make the code more compact. At first it would be useful the avoid the repetition in the if and else if branches. Additionally, I would suggest to only have one parameter for each corner, maybe as superclass?

Furthermore, I would appreciate if you would adapt the same code style I used. In particular putting else blocks on a new line.

Thank you!

@swoolcock
Copy link
Author

swoolcock commented Apr 9, 2017

My idea at the moment is to create an empty protocol (such as DecorationViewType) and put extensions on UINib and AnyClass to make them conform to it. Then the init can again take one parameter per corner. The parameter names would need to change though, since they're no longer just nibs. I'd prefer not to make a breaking change if possible.

I'm hesitant to just overload init with one that only takes AnyClass?, because the developer may want to mix nibs and classes. This might be a temporary solution, though.

Edit:
AnyClass is a metatype and can't have extensions. I'll just add an overloaded convenience init for now, then.

Edit 2:
As I feared, duplicating the initializer is not a simple solution, since it creates an ambiguous call if you only pass the delegate. The safest/simplest solution I can think of is to add 4 parameters to init as I have, but clean up the duplicated code. Otherwise we risk breaking backward compatibility.

Copy link
Owner

@stuffrabbit stuffrabbit left a comment

Choose a reason for hiding this comment

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

Hey,
thank you again for the contribution. The code looks much better now. However, I would suggest to only pass one parameter per DecorationView "corner" as type Any, then to check wether if its of type AnyClass or UINib and finally call the respective registration method. What do you think?

@swoolcock
Copy link
Author

My concerns with that approach are:

  1. The developer could pass something other than UINIb or AnyClass, so it would have to either ignore it, and/or output a runtime error.
  2. Combining the initializer arguments would mean renaming them, which breaks backward compatibility.

If you're ok with both of these, I'll make the change.

@stuffrabbit
Copy link
Owner

To your concerns:

  1. Yes thats true but I think a log warning in case of a wrong class and additional documentation for this method should be sufficient to address this issue. I think this is still better than additional parameters.

  2. You are right, but it is just a minor change though so people will be able to handle that :)

It would be great if you could update the README and the demo application as well.
Thank you!

@stuffrabbit
Copy link
Owner

@swoolcock this is addressed in update 1.2.0:
I am using an enum as parameter which either holds a nib or class as associative value.

@stuffrabbit stuffrabbit closed this Jun 9, 2018
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