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 option to include section headers in animations #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaxHaertwig
Copy link

This code adds two static properties to Spruce, providing the option to include section headers of UITableView or UICollectionView in Spruce animations.

Usage (anywhere in app):
Spruce.includeTableViewSectionHeaders = true
Spruce.includeCollectionViewSectionHeaders = true
// The default value is false for both properties (current behavior)

@jacksontaylor13
Copy link
Contributor

@Vyax First thank you very much for the contribution! Is there a reason why you chose to make the variables static?

My thinking is that maybe we should add such functionality to the animate and prepare functions so that on certain pages you can include the headers while on others you won't.

@MaxHaertwig
Copy link
Author

The animate and prepare functions already take a lot of parameters, so I though having static variables would be a good solution. They can also be set in viewDidLoad of each individual screen.

Do you want me to add them as parameters to animate?

jacksontaylor13 added a commit to jacksontaylor13/spruce-ios that referenced this pull request Jan 29, 2018
Looks like the parameters were getting too large for Spruce. Decided to try and have it with AnimationOptions instead so that the method length is customized based off of how much you need. This way it will be easier to read and add more options in the future like PR willowtreeapps#95.

Also added in the documentation for the new features
@jacksontaylor13
Copy link
Contributor

@Vyax I completely agree! They definitely do already take too many parameters. I just added a PR that will swap parameters over for AnimationOption values. This way we can keep adding parameters without worrying about the method schema length. We'll see if the maintainers like it and then maybe we can transition your PR to use the options layout instead.

@zackdotcomputer
Copy link
Contributor

I would love to see a PR adding functionality like this merged in, though I agree with @jacksontaylor13 that it would be best to do so as a per-animation configuration rather than using static variables at the top level.

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.

3 participants