-
Notifications
You must be signed in to change notification settings - Fork 556
feat: save location name in a shared preference #172
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
Conversation
| private val eventsViewModel by viewModel<EventsViewModel>() | ||
| private lateinit var rootView: View | ||
| private val preference: Preference = Preference() | ||
| private val tokenKey = "LOCATION" |
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.
There's a reason to use MVVM. Please don't add logic in Views
|
@iamareebjamal moved entire preference logic to ViewModel. |
| private val eventsRecyclerAdapter: EventsRecyclerAdapter = EventsRecyclerAdapter() | ||
| private val eventsViewModel by viewModel<EventsViewModel>() | ||
| private lateinit var rootView: View | ||
| private val preference: Preference = Preference() |
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.
No logic in fragment
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.
@iamareebjamal I moved the logic to viewmodel in this commit 0636e28
but the tests were failing so I reverted.
Was it the correct way?
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
|
@iamareebjamal I tried to fix the failing dependency test but I wasn't able Could you please help |
| val error = MutableLiveData<String>() | ||
|
|
||
| var locationName: String? = null | ||
| var locationName: String? = preference.getString(tokenKey) |
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.
Make this lazy, it'll work
| var locationName: String? = null | ||
| var locationName: String? = preference.getString(tokenKey) | ||
|
|
||
| fun loadLocationEvents() { |
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.
Why not take the location name as parameter
|
@iamareebjamal Did all the requested changes please review |
| class EventsViewModel(private val eventService: EventService, private val preference: Preference) : ViewModel() { | ||
|
|
||
| private val compositeDisposable = CompositeDisposable() | ||
| val tokenKey = "LOCATION" |
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.
private val
iamareebjamal
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.
RC
|
PR updated, please review |
fixes #159

Now events can be loaded automatically when the user opens the app