-
Notifications
You must be signed in to change notification settings - Fork 0
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
Keyword filter #56
Keyword filter #56
Conversation
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.
Looks great! One minor nitpick for the OnClick listener functions.
Also, a reoccurring issue with the FilterFragments as a whole that I've noticed while playing around is that if you click apply with nothing set, the whole list disappears. We should probably add something that either prevents this in the fragment or just keeps the whole list there if nothing is applied.
app/src/main/java/com/example/househomey/filter/ui/KeywordFilterFragment.java
Outdated
Show resolved
Hide resolved
@owencooke @antonio2uofa we can tackle this in a separate [BUGFIX] PR |
Already done. |
Fix for the empty keywords works beautiful @antonio2uofa 🙌 |
got the last cheeky bugger. Should be all lambdas and clear skies |
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.
Looks great! I have made a branch, and a PR with a couple minor changes that take advantage of a couple things in Java. If you are cool with them just approve that PR and merge it into your branch
app/src/main/java/com/example/househomey/filter/ui/KeywordFilterFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/househomey/filter/ui/KeywordFilterFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/househomey/filter/ui/KeywordFilterFragment.java
Outdated
Show resolved
Hide resolved
* PR recommendations * PR recommendations
@ldbonkowski I squashed and merged! |
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!
Describe your changes
This PR accomplishes the following:
Issue ticket number and link
This PR addresses: #25 and #21
Checklist before requesting a review
Screenshots (if applicable)