-
Notifications
You must be signed in to change notification settings - Fork 80
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
PIA-1003: Sort and filter servers list by favorite and recommended #70
Conversation
e1fb395
to
e916d98
Compare
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.
Great job @kp-laura-sempere! Just a couple of comments that we can discuss tomorrow in a session.
} | ||
} | ||
|
||
extension Client: ClientType { |
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 think I have not seen it being used and we already have ClientTypeImpl, do we need to make it conform to the procotol?
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.
Right, I created it to do the refresh latency but we are not using it.
I will remove it and then create a ticket to handle the refresh latency when needed
} | ||
} | ||
|
||
private func refreshRegionsList() { |
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.
It seems that this ViewModel is probably doing many things, maybe we can break it down a bit, what do you think?
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 @kp-said-rehouni ,
Yes, the thing is that we need to perform a lot of things from the regions list: add and remove to favorites, sorting by name and by recommended and search.
I created various UseCases to handle getting the servers list and also adding and removing from favorites. And this VM makes use of those use cases to do that. But in terms of sorting, filtering and searching (which it's another kind of filtering), not sure how we can split it out of this VM.
Do you have any suggestions? I will have another look and see if I can think of ways to do it
d776e79
to
1568738
Compare
e434530
to
56c93d5
Compare
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! Great job @kp-laura-sempere!
This PR includes search, sort and filter functionatities: