-
Notifications
You must be signed in to change notification settings - Fork 9
Auto-scroll to the current/next session in Programme #2
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
base: master
Are you sure you want to change the base?
Conversation
|
If you like this change we can do the same for My Schedule too. |
JoachimK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @marcpalmer, that's a feature I also wanted to have!
I left a few ideas for improvements/simplifications, let me know whether these make sense to you. :-)
iOSDevUK/Controllers/Sessions/ProgrammeTableViewController.swift
Outdated
Show resolved
Hide resolved
iOSDevUK/Controllers/Sessions/ProgrammeTableViewController.swift
Outdated
Show resolved
Hide resolved
| let session = $0 as! Session | ||
| return session == sessionToScrollTo | ||
| }) { | ||
| tableView.scrollToRow(at: IndexPath(row: rowIndex, section: sectionIndex), at: .top, animated: animated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using .top here or the default .none? .none would not scroll it if it's already visible e.g. at the bottom of the screen, it would only make sure that it's visible at all. I'm assuming that this is nicer, as it reduces unnecessary scrolling, but we'd probably have to try it out in the app to see how it feels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, try it out. I did... it felt good - you just pull down if you wanted to check the previous session but... IMO a very rare case?
| } | ||
|
|
||
| /// Find the session in the already-fetched results | ||
| let nextOrCurrentSession = fetchedResultsController.fetchedObjects?.first { session in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that the Session object already has a way to calculate the current and the next Session. Maybe you can use Session.nowSession(forDate:) and Session.nextSession(forDate:) here instead of reimplementing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw those too, but it seems like a silly idea to run 2 Core Data queries to find this?
This was something I kept wishing the app would do, so I made it.
It's probably hacky/not the way you would do it knowing the code better but it seems reasonable to me e.g. a new CoreData query for this rather than scanning the fetched objects seems overkill given we already have the sessions loaded.
Challenge: try to find a way to scroll to the current session without animating the scroll when the VC first loads. I can't find a time when the tableview has fully laid out for the auto-sizing cells and the wrapping text. Everything I tried meant it scrolled to the wrong location (more noticeable later in the day and the longer the session item names are) - because on first display it still thinks the layout is the estimated size. Help?