-
Notifications
You must be signed in to change notification settings - Fork 471
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(web): add support for unrated results in RatingsFilter #1003
Conversation
Hey, @ShahAnuj2610 Thanks for the PR. Have you tested this by keeping |
@jyash97 fixed! |
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.
The feature works well but there are some UI changes and testing needed while implementing this. Here are a few things that I noticed:
-
When I don't have
includeUnrated
, I see an extra comma in the tab. Check this:
-
When we don't have
includeUnrated
the URL params just saystrue
ornull
so either we should showincludedUnrated
or doesn't show. cc @siddharthlatest Thoughts? -
There is a proptype error coming on selectedValue
-
We will also need to document these changes over here
For 2., we don't need a URL query string for includeUnrated.
…-Siddharth
On Fri, May 31, 2019, 11:39 Yash Joshi ***@***.***> wrote:
***@***.**** commented on this pull request.
@ShahAnuj2610 <https://github.com/ShahAnuj2610>
The feature works well but there are some UI changes and testing needed
while implementing this. Here are a few things that I noticed:
1.
When I dont have includeUnrated, I see an extra comma in the tab.
Check this:
[image: image]
<https://user-images.githubusercontent.com/22376783/58684873-65a31e00-8397-11e9-84de-6ec19056dfca.png>
2.
When we don't have includeUnrated the URL params just says true or null
so either we should show includedUnrated or doesn't show. cc
@siddharthlatest <https://github.com/siddharthlatest> Thoughts ?
3.
There is a proptype error coming on selectedValue
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1003?email_source=notifications&email_token=AAEZ2GVQJOYBW3WAPXY5PF3PYC6JRA5CNFSM4HQPA4LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2GWEWQ#pullrequestreview-244146778>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEZ2GVKUXZZGWRQIVHNE63PYC6JRANCNFSM4HQPA4LA>
.
|
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.
#267