-
-
Notifications
You must be signed in to change notification settings - Fork 741
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
reset selected filters after search #5033
base: development
Are you sure you want to change the base?
reset selected filters after search #5033
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.
@PikachuEXE I'm attaching a test video with the mentioned steps and cannot replicate the issue. Please share similar so I can fix it ASAP and the same happens in YouTube (as I feel we're taking references from there). Like even if we hit search for the same query it resets the filter. Not sure, If we've a different requirement here. Screen.Recording.2024-04-29.at.10.09.26.AM.mov |
Screen.Recording.2024-04-29.at.13.33.20.mov |
Head branch was pushed to by a user without write access
@PikachuEXE I've implemented a check, not to reset filter for the same query. |
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.
Filter not applied when:
- Application starts
- apply labels
- enter query
- hit search
VirtualBoxVM_E7hDKoPbOO.mp4
Head branch was pushed to by a user without write access
f754ae7
to
ddeb438
Compare
Fixed. Thanks for the review. |
It reset filters under strange conditions (e.g. like changing filter below Screen.Recording.2024-04-30.at.14.24.09.movProbably like
|
@PikachuEXE this is not a bug but intended behavior. There is how YT also behaves. In your example Time Today resets because you clicked on Type Channel. Time is based on upload date. A channel cant have a upload date. |
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.
OK there are several issue discovered during testing but not introduced by this PR
Issue? |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Head branch was pushed to by a user without write access
Conflicts have been resolved. A maintainer will review the pull request shortly. |
@jasonhenriquez can we get this reviewed 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.
Looks like the implementation was changed enough by #3975 to make the required alteration easier. Address review comments, re-validate, and you should be good.
src/renderer/components/ft-search-filters/ft-search-filters.vue
Outdated
Show resolved
Hide resolved
Hi @ankitchauhan-aka, are you still available to make these final alterations? |
Hi @jasonhenriquez |
…into ankitchauhan-aka/reset-filter-after-search
Head branch was pushed to by a user without write access
@jasonhenriquez seems good now! |
In my testing, it's not actually applying the filters now - the probable issue is that the removal is processing before the results are being grabbed. |
I think that's correct behaviour. It should remove filter if search query is modified. |
No, I mean it's not applying the filter even the first time. |
https://www.awesomescreenshot.com/video/28282864?key=3f195c5137a0c69969519b2bdf7fe124 |
@jasonhenriquez could you please share! |
Hi @ankitchauhan-aka, sorry for the delay. Just retested now; it was working the first handful of times, then just stopped applying the filter at all. screen-recording.mp4Before moving forward with this, I'd also like some discussion on the intended behavior. We're showing the filter as active after a search is made, until the user submits another query. That's not intuitive, as we shouldn't be showing the filter as if it's active if it isn't going to be. The main reason YT has this behavior the way it is is because changing a filter setting automatically re-submits the same search query but with the new filter. If we do want this feature, the filters should show as clear immediately after the search processes. |
Head branch was pushed to by a user without write access
@jasonhenriquez thanks for the video, I can see the issue in your video but am not able to reproduce it. Did you notice any steps for the issue? Also, I've modified it for the new requirement to clear the filter after search |
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. The bug that I was encountering was setting a feature filter while a channel was set, which should be fixed by #5197.
@jasonhenriquez great. Hoping to get it merged before we get any conflicts again 😉 |
Pull Request Type
Related issue
closes #1693
Description
Testing