-
Notifications
You must be signed in to change notification settings - Fork 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
feat: add interval refresh on the sks people counter #415
feat: add interval refresh on the sks people counter #415
Conversation
@tomasz-trela if u have enought time, pls perform a review and ping me when done, I will check it out as well ;) |
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.
Everything looks good besides code duplication. Right now I can't test if it is working, but code looks good.
extension RefIntervalRefreshX on Ref { | ||
void setRefresh(Duration interval) { | ||
final timer = Timer.periodic( | ||
interval, | ||
(t) => invalidateSelf(), | ||
); | ||
onDispose(timer.cancel); | ||
} | ||
} | ||
|
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.
We have this part of code in file parkings_repository.dart so my idea is to move this to separate file(like ref_extesions.dart) in utils folder and use it in 2 places.
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.
Sure thing! I can take care of refactorization. By the way - to test code it is nessesary to add SksAppBar somewhere. It wasn't placed anywhere before so I left it as it was.
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.
You can add it at app bar at main screen (for testing purposes)
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.
good catch Tomuś
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.
Good Work! Nice use of properly named extension! Minor adjustment of mine and one declared by Tomek
final currentTime = DateTime.now(); | ||
final sksRefreshInterval = sksData.isResultRecent | ||
? sksData.nextUpdateTimestamp.difference(currentTime) | ||
: const Duration(minutes: 1); |
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.
Please extract it to some const, eg. in config (duration time)
return SksUserData.fromJson(response.data as Map<String, dynamic>); | ||
final sksData = SksUserData.fromJson(response.data as Map<String, dynamic>); | ||
|
||
final currentTime = DateTime.now(); |
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.
Just a suggestion, please check DateTimeUtils
. In mentioned file Simon created handy extension now
. Maybe it'll suit you here as well
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.
tbh this can stays as it is (imo)
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 agree, placed it here as fun fact lets say
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.
good job guys
extension RefIntervalRefreshX on Ref { | ||
void setRefresh(Duration interval) { | ||
final timer = Timer.periodic( | ||
interval, | ||
(t) => invalidateSelf(), | ||
); | ||
onDispose(timer.cancel); | ||
} | ||
} | ||
|
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.
good catch Tomuś
return SksUserData.fromJson(response.data as Map<String, dynamic>); | ||
final sksData = SksUserData.fromJson(response.data as Map<String, dynamic>); | ||
|
||
final currentTime = DateTime.now(); |
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.
tbh this can stays as it is (imo)
add files: sks_people_live_consts.dart ref_extensions.dart change files: home_viev.dart: add SksAppBar to AppBar parkings_repository.dart, latest_sks_user_data_repository.dart: refactor code
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.
LGTM!
* feat: add interval refresh on the sks people counter * refactor: add separate files for refresh on ref and consts add files: sks_people_live_consts.dart ref_extensions.dart change files: home_viev.dart: add SksAppBar to AppBar parkings_repository.dart, latest_sks_user_data_repository.dart: refactor code * refactor: delete unused imports
lastest_sks_user_data_repo.dart: add interval refresh
sks_user_data.dart: refactor model attributes to match API responses