Skip to content

Add cell deselection API#853

Closed
rnystrom wants to merge 3 commits intomasterfrom
deselect
Closed

Add cell deselection API#853
rnystrom wants to merge 3 commits intomasterfrom
deselect

Conversation

@rnystrom
Copy link
Copy Markdown
Contributor

@rnystrom rnystrom commented Jul 12, 2017

Changes in this pull request

Adding support for a cell deselection API. Trying to make some headway to move and drag+drop support, but also want better stock UICollectionView API support. Will also assist eventual UITableView support.

  • Added overridable API to IGListSectionController
  • Support for stacked SC
  • Breaking, required protocol for binding SC

Assists #524 and #184

Checklist

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.

@weyert
Copy link
Copy Markdown
Contributor

weyert commented Jul 12, 2017

Looks alright to me. I see you made a default implementation for this method

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@rnystrom updated the pull request - view changes

@jessesquires jessesquires modified the milestone: 3.1.0 Jul 12, 2017
@jessesquires
Copy link
Copy Markdown
Contributor

3.1.0 cannot contain breaking changes according to SemVer, we'll need to make this 4.0.0. 😊

I think this is fine though. People tend to freak out about version numbers, I don't think it's a big deal. What's important is communicating changes + proper SemVer.

Looks like there's a handful of things in the 3.1.0 milestone that we could wrap up -- we could merge those and this, rename to 4.0 and push a release. The current 4.0 would then get bumped to 5.0.

Thoughts?

cc @amonshiz

@rnystrom
Copy link
Copy Markdown
Contributor Author

rnystrom commented Jul 12, 2017

@jessesquires TIL! Another option: I could make this with @optional then remove for 4.0? Not opposed to a bigger 4.0 release either.

Or maybe I hold off on this, land some internal changes + fixes (I have some diffs up inside), release 3.1, then land + prep 4.0?

edit: The @optional would only effect IGListBindingSectionController which is a small part of this feature

@amonshiz
Copy link
Copy Markdown
Contributor

i'm fine bumping table view support to 5.0

@jessesquires
Copy link
Copy Markdown
Contributor

@rnystrom -- either of those sound good to me.

RE: 4.0 release -- We could just merge this and immediately push 4.0, then anything else goes into 4.1. I was just thinking, since we have some low-hanging fruit, we could include more in a 4.0 release to make it "more worth it"

@rnystrom
Copy link
Copy Markdown
Contributor Author

rnystrom commented Jul 12, 2017

Ya I'd rather package a bit more into the 4.0 since semantically its kind of a big deal. Atm there's barely anything compared to 3.0.

I lean towards @optional and then remove that for 4.0

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@rnystrom updated the pull request - view changes

@Instagram Instagram deleted a comment from facebook-github-bot Jul 13, 2017
@rnystrom rnystrom requested a review from jessesquires July 13, 2017 00:30

@note Method is `@optional` until the 4.0.0 release where it will become required.
*/
@optional
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i thought swift didn't support @optional which is why there has been a push internally to remove all @optional protocol methods and properties.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Still supports it, discouraged b/c of how it will silently break things

https://useyourloaf.com/blog/swift-optional-protocol-methods/

@rnystrom
Copy link
Copy Markdown
Contributor Author

Importing for eyeballs

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jessesquires
Copy link
Copy Markdown
Contributor

@rnystrom sounds good to me 👍

let's make sure we create a 4.0 task to remove @optional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants