Introduce a freshness indicator for the goal screen #537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theospears
reviewed
Dec 6, 2024
krugerk
force-pushed
the
feature/246-loading-indicator-for-goal
branch
from
660661c to
1228e01
Compare
krugerk
force-pushed
the
feature/246-loading-indicator-for-goal
branch
from
1228e01 to
6616f94
Compare
theospears
requested changes
Dec 27, 2024
There was a problem hiding this comment.
Overall approach looks good, and I like the idea of extracting FreshnessIndicatorView as its own component. I added some requests for local changes.
When testing, could you also verify the time since update changes when you just leave the app open with the goal view showing?
| } | ||
|
|
||
| self.updateLastUpdatedLabel() | ||
| Timer.scheduledTimer(timeInterval: 60, target: self, selector: #selector(GoalViewController.updateLastUpdatedLabel), userInfo: nil, repeats: true) |
There was a problem hiding this comment.
[Consider] Wouldn't this timer make more sense being part of the FreshnessIndicatorView ?
There was a problem hiding this comment.
The reason I placed it here is because this is also a Controller whereas the Freshness one is just a view.
krugerk
force-pushed
the
feature/246-loading-indicator-for-goal
branch
from
01efea0 to
67b1c7a
Compare
theospears
requested changes
Dec 29, 2024
a variation of _a loading indicator for goals_
DRY the view showing both the gallery's and a goal's freshness (last fetched date)
krugerk
force-pushed
the
feature/246-loading-indicator-for-goal
branch
from
67b1c7a to
b3d893c
Compare
This reverts commit 1f3af669fdc5296092b8df4eba0d3d7f855ee226. removing a commit that did not directly contribute to the solution
krugerk
force-pushed
the
feature/246-loading-indicator-for-goal
branch
from
5975456 to
d68cdbd
Compare
krugerk
commented
Dec 29, 2024
Comment on lines
35
to
30
| fileprivate var goalImageView = GoalImageView(isThumbnail: false) | ||
| fileprivate var datapointTableController = DatapointTableViewController() | ||
| fileprivate var dateTextField = UITextField() |
There was a problem hiding this comment.
Suggested change
| fileprivate var goalImageView = GoalImageView(isThumbnail: false) | |
| fileprivate var datapointTableController = DatapointTableViewController() | |
| fileprivate var dateTextField = UITextField() | |
| fileprivate let goalImageView = GoalImageView(isThumbnail: false) | |
| fileprivate let datapointTableController = DatapointTableViewController() | |
| fileprivate let dateTextField = UITextField() |
Comment on lines
27
to
28
| var deadbeatView = UIView() | ||
| var outofdateView = UIView() |
There was a problem hiding this comment.
Suggested change
| var deadbeatView = UIView() | |
| var outofdateView = UIView() | |
| private let deadbeatView = UIView() | |
| private let outofdateView = UIView() |
theospears
approved these changes
Dec 29, 2024
|
LGTM, merged! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In line with #246, this provides an indicator of freshness on the goal screen. It reuses the one from the gallery screen.
Media
last.fetch.on.gallery.and.goalvc.mp4
Validation
Ran app in simulator
Viewed both gallery and goal, navigating back and forth.
Sent the app to background, back into foreground, varying sometimes with the gallery open, sometimes with a goal open.
Also watched the last updated since label be updated while just leaving the app open with the goal view showing
Fixes #246