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

Calendar Support #229

Merged
merged 27 commits into from Dec 23, 2017
Merged

Calendar Support #229

merged 27 commits into from Dec 23, 2017

Conversation

SoneeJohn
Copy link
Contributor

@SoneeJohn SoneeJohn commented Jun 7, 2017

I noticed today while I was viewing sessions on my computer that there is no support for quickly adding a session to the calendar. The officially WWDC app has this so I thought I should add it.

By the way, thanks for the awesome app @insidegui 🙂

TODO:

  • Support for adding session to the calendar

  • Only show calendar button when WWDC is active

  • Store eventIdentifier somewhere so it can be referenced later so the user can remove the event from the app

@insidegui
Copy link
Owner

Thanks @SoneeJohn ! I'll review this as soon as I can

@SoneeJohn
Copy link
Contributor Author

@insidegui I was planning to save the session identifier along with the eventIdentifier of EKEvent so that the user can remove it from their calendar. However, I was wondering if that would be too much.

@insidegui
Copy link
Owner

@SoneeJohn you can add that as a property to SessionInstance. Don't forget to increase the coreSchemaVersion in Constants.swift so the database gets migrated correctly. If you don't know how to work with Realm, you can find the documentation here: https://realm.io/docs/swift/latest/

@SoneeJohn
Copy link
Contributor Author

@insidegui thanks! I'll look into this tomorrow.

@insidegui
Copy link
Owner

@VicenteBorrell Can you get us an icon for the "add to calendar" feature? It will only be useful next year, but nice to have :)

@VicenteBorrell
Copy link
Collaborator

Yeah, no problem! I was thinking, maybe we could leave it for next year. I would like to improve the session detail layout, adding location and time, and put the icon next to it.

@SoneeJohn
Copy link
Contributor Author

@insidegui finished :-) All what might be needed now is a new calendar icon from @VicenteBorrell

@insidegui
Copy link
Owner

@VicenteBorrell Since @SoneeJohn has already done all the hard work of implementing calendar support, I don't think we should wait until next year to merge this. Can you get us an icon for the feature?

Thanks

@VicenteBorrell
Copy link
Collaborator

You can find the icon on the Sketch file @insidegui
Here it is too:
calendar icon 2x

@SoneeJohn
Copy link
Contributor Author

I updated the icon. Thanks @VicenteBorrell !

@insidegui
Copy link
Owner

@SoneeJohn I made some changes to the implementation and migrated it to our code style and recent changes. You can see the updated branch here: https://github.com/insidegui/WWDC/tree/calendar

Can you make it so the calendar button only appears for sessions that are in the future? There's no point in showing it for sessions that have already passed.

@SoneeJohn
Copy link
Contributor Author

@insidegui I see @bcmn beat me to it ;-) 5c14187

@bcmn
Copy link
Collaborator

bcmn commented Oct 25, 2017

Oh oops, I thought I’d commented here, sorry. 😬 Gui tried to prod me too after I added it just from seeing activity on the branch. 😂 Yeah, it’s currently based on the endTime of the session, which seems like it allows a bit more buffer for edge use cases, I don’t know if anyone disagrees.

@SoneeJohn
Copy link
Contributor Author

@bcmn No worries 😂. I think it would be better if the button simply don’t show up if the startTime is now in the past.

@bcmn
Copy link
Collaborator

bcmn commented Oct 25, 2017

Yeah, I think you're right. I think I wavered when deciding and then made the choice based on an incorrect memory of how much of the event details we surface in UI. I changed it.

@insidegui
Copy link
Owner

Yeah I agree, there's no point in adding an event that has already started to your calendar ¯_(ツ)_/¯

I'll do some more testing tonight and merge after reviewing the changes.

Thanks

@bcmn bcmn merged commit 1ea099a into insidegui:master Dec 23, 2017
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

4 participants