Skip to content
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

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

ankitchauhan-aka
Copy link

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #1693

Description

Testing

  1. Apply filters, You must have a red border that shows filter(s) are selected
  2. Then search for a term in the search box and choose any option from autosuggestion dropdown
  3. After selecting autosuggested options, Filters must be reset to default options.
  4. The red border from the filter icon will be removed

@ankitchauhan-aka ankitchauhan-aka marked this pull request as ready for review April 28, 2024 09:18
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 28, 2024 09:18
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 28, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with these steps and failed

  • Enter something, enter search result via enter/search button
  • Change filter options
  • Press search button again
  • Search result not updated but filter reset

image

@ankitchauhan-aka
Copy link
Author

ankitchauhan-aka commented Apr 29, 2024

I tested with these steps and failed

  • Enter something, enter search result via enter/search button
  • Change filter options
  • Press search button again
  • Search result not updated but filter reset

image

@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

@PikachuEXE
Copy link
Collaborator

Screen.Recording.2024-04-29.at.13.33.20.mov

auto-merge was automatically disabled April 29, 2024 14:11

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 29, 2024 14:11
@ankitchauhan-aka
Copy link
Author

ankitchauhan-aka commented Apr 29, 2024

@PikachuEXE I've implemented a check, not to reset filter for the same query.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filter not applied when:

  1. Application starts
  2. apply labels
  3. enter query
  4. hit search
VirtualBoxVM_E7hDKoPbOO.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 29, 2024
auto-merge was automatically disabled April 30, 2024 04:47

Head branch was pushed to by a user without write access

@ankitchauhan-aka ankitchauhan-aka force-pushed the ankitchauhan-aka/reset-filter-after-search branch from f754ae7 to ddeb438 Compare April 30, 2024 04:47
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 30, 2024 04:48
@ankitchauhan-aka
Copy link
Author

ankitchauhan-aka commented Apr 30, 2024

Filter not applied when:

  1. Application starts
  2. apply labels
  3. enter query
  4. hit search

VirtualBoxVM_E7hDKoPbOO.mp4

Fixed.

Thanks for the review.

@PikachuEXE
Copy link
Collaborator

It reset filters under strange conditions (e.g. like changing filter below

Screen.Recording.2024-04-30.at.14.24.09.mov

Probably like

  • Search A
  • Enter B
  • Try to set multiple criteria

@efb4f5ff-1298-471a-8973-3d47447115dc

@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.

PikachuEXE
PikachuEXE previously approved these changes Apr 30, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a 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

@efb4f5ff-1298-471a-8973-3d47447115dc

Issue?

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Apr 30, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 1, 2024
auto-merge was automatically disabled May 7, 2024 16:31

Head branch was pushed to by a user without write access

Copy link
Contributor

github-actions bot commented May 7, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@ankitchauhan-aka
Copy link
Author

ankitchauhan-aka commented May 7, 2024

@jasonhenriquez can we get this reviewed now?

Copy link
Collaborator

@jasonhenriquez jasonhenriquez left a 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/top-nav/top-nav.js Outdated Show resolved Hide resolved
@jasonhenriquez
Copy link
Collaborator

Hi @ankitchauhan-aka, are you still available to make these final alterations?

@ankitchauhan-aka
Copy link
Author

ankitchauhan-aka commented May 26, 2024

Hi @jasonhenriquez
Yeah. I was too much occupied in my job, will do it asap

auto-merge was automatically disabled June 2, 2024 15:27

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 2, 2024 15:27
@ankitchauhan-aka
Copy link
Author

@jasonhenriquez seems good now!

@jasonhenriquez
Copy link
Collaborator

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.

@ankitchauhan-aka
Copy link
Author

I think that's correct behaviour. It should remove filter if search query is modified.

@jasonhenriquez
Copy link
Collaborator

No, I mean it's not applying the filter even the first time.

@ankitchauhan-aka
Copy link
Author

https://www.awesomescreenshot.com/video/28282864?key=3f195c5137a0c69969519b2bdf7fe124
@jasonhenriquez Please take a look at the attached video, if you feel that some of the records do not match the time filter, So it's not related to our application logic, Even YouTube has the same results. or if your testing produces different results than mine please share a capture.

@ankitchauhan-aka
Copy link
Author

@jasonhenriquez could you please share!

@jasonhenriquez
Copy link
Collaborator

jasonhenriquez commented Jun 6, 2024

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.mp4

Before 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Active filter must reset after i search for something else
4 participants