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

Added example schemes for Today/iMessage extensions #112

Closed
wants to merge 8 commits into from

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Oct 23, 2016

Changes in this pull request

Offering this open to a bit of a discussion based on the comments at #77.

  • Created two new schemes (IGListKitTodayExample, IGListKitMessageExample) in which respectively correspond to their app extension.
  • Updated the Podfile to ensure they have IGListKit as a dependency
  • Added a super simple example reusing the LabelSectionController from the main app

Done more as a 'yes it can be done' as opposed to 'look at what can be done' hence the basic functionality but I can see ways to improve if necessary!

Pull request checklist

@Sherlouk
Copy link
Contributor Author

Just as a secondary note I'm not sure how many of these changes are because it's a new scheme, I have a different version of CocoaPods or I simply made a mistake - revise checking! (Not big with CP)

Also my justification for iOS 10 deployment target is because of the OS changes, Apple completely revamped both of them in iOS 10 so they're changed a fair bit (Can't even do iMessage extension in older versions, though strangely it's allowed?). However, if it is desired that they match the deployment target of the library then I'm happy to make these changes!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 24, 2016

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

edit: hit the wrong button, not actually importing this yet

@Sherlouk
Copy link
Contributor Author

@rnystrom I'm not sure how you guys would like to treat the extra schemes, whether you want a demo chooser similar to the main app or whether you want one example which shows that it's possible (probably with a bit more functionality than just a few cells) and leave the alternative examples to the main app?

A bit of steer would be appreciated!

cc: @jessesquires

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@coveralls
Copy link

coveralls commented Oct 26, 2016

Coverage Status

Coverage remained the same at 86.082% when pulling cdc0717 on Sherlouk:multiple-schemes into 8855b51 on Instagram:master.

@rnystrom
Copy link
Contributor

@Sherlouk Ok finally got a chance to poke around at this, and WOW. This is so cool! 🚀

Some organizational nits:

  • MessageExtension dir should be IGListKitMessageExample
  • Same renaming for the iMessage target (haven't made iMessage apps before, hopefully this is simple)
  • Rebase and use the icons from 920aad6
    • Idk if we need new assets, if so I have them all I can upload here
  • Bundle display name to "IGListKit" for all of them

@jessesquires check this out. cray:

simulator screen shot oct 27 2016 12 36 00 pm

simulator screen shot oct 27 2016 12 36 25 pm

@jessesquires
Copy link
Contributor

that's awesome 🎉

@jessesquires
Copy link
Contributor

should we add this to the changelog?

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@rnystrom
Copy link
Contributor

@jessesquires good call, let's do that. cc @Sherlouk

@Sherlouk
Copy link
Contributor Author

@rnystrom Thanks! ❤️
I've updated the display names to IGListKit, and refactored the scheme name & directory to match the target.

The "Today Extension" icon comes from the base app, but annoyingly the message app icon is completely different (they're slightly rectangular) so you can't use the same ones as the main app! Maybe a job for your design guys?

Just seen the message about the changelog, I'll update that now!

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@Sherlouk
Copy link
Contributor Author

Wasn't sure whether to just add to the bottom (as I did) or to add a new section "Example Pack Changes" which can be specifically for changes to the example pack to distinguish against modifications to the actual library.

Happy to modify upon feedback!

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@Sherlouk
Copy link
Contributor Author

There was something deep down inside me which meant I just had to move that storyboard once I saw it out of place! 😅

Also RIP travis?

@@ -18,6 +18,7 @@ This release closes the [2.0.0 milestone](https://github.com/Instagram/IGListKit
- Fixed `-[IGListAdapter reloadDataWithCompletion:]` not returning early when `collectionView` or `dataSource` is nil and `completion` is nil. [Ben Asher](https://github.com/benasher44) [(#51)](https://github.com/Instagram/IGListKit/pull/51)
- Added `-isFirstSection` and `-isLastSection` APIs to `IGListSectionController`
- Added support for cells created from storyboard. [Bofei Zhu](https://github.com/zhubofei) [(#92)](https://github.com/Instagram/IGListKit/pull/92)
- Added examples for Today & iMessage extensions. [Sherlouk](https://github.com/Sherlouk) [(#112)](https://github.com/Instagram/IGListKit/pull/112)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jessesquires
Copy link
Contributor

ha 😄 travis should be working ok (but will probably take awhile)

@Sherlouk
Copy link
Contributor Author

Are we happy with the overall complexity of these examples?

Started with very basic functionality, but not sure if I should do something a bit more exciting! Open to suggestions there, or just to leave it as is

@jessesquires
Copy link
Contributor

@Sherlouk - I think this is good for this PR. Feel free to open a new issue to discuss! 😄

@rnystrom
Copy link
Contributor

Simple is good! More a proof of concept that this is possible w/ fairly minimal setup.

The other thing this shows off is the fact that you can build a single section controller and share it between different targets/implementations! Pretty slick.

@jessesquires should we make the LabelSectionController class that all 3 examples use live in a shared lib? Or is having the message and today widget use a section controller from the main example app ok?

@Sherlouk
Copy link
Contributor Author

While in a full application I'd probably agree with your suggestion there Ryan, I actually think in an example pack like this it's much easier to follow the chain of target membership if you can simply see the section controller can be used in multiple targets.

Just my two cents :)

@jessesquires
Copy link
Contributor

yeah, agree with @Sherlouk 💯

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage remained the same at 86.194% when pulling 8fd998a on Sherlouk:multiple-schemes into b99776e on Instagram:master.

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage remained the same at 86.194% when pulling 8fd998a on Sherlouk:multiple-schemes into b99776e on Instagram:master.

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage remained the same at 86.194% when pulling 8fd998a on Sherlouk:multiple-schemes into b99776e on Instagram:master.

@Sherlouk
Copy link
Contributor Author

Builds are done! 🎉 🎉

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@coveralls
Copy link

coveralls commented Oct 28, 2016

Coverage Status

Coverage remained the same at 86.194% when pulling 3f6524c on Sherlouk:multiple-schemes into eca0205 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes - changes since last import

@coveralls
Copy link

coveralls commented Oct 29, 2016

Coverage Status

Coverage increased (+0.3%) to 86.482% when pulling c021251 on Sherlouk:multiple-schemes into e4a4719 on Instagram:master.

@Sherlouk
Copy link
Contributor Author

Not at all sure how I've increased unit tests 🤔

@rnystrom We waiting on anything specific to progress this?

@rnystrom
Copy link
Contributor

@Sherlouk nope! Landing internally right now

@Sherlouk Sherlouk deleted the multiple-schemes branch December 30, 2016 14:09
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