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.

Sign up for GitHub

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

Jump to bottom

reset selected filters after search #5033

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

reset selected filters after search #5033

wants to merge 3 commits into from
+31 −1

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
PikachuEXE
PikachuEXE requested changes Apr 29, 2024
View reviewed changes
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 •
edited

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 •
edited

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

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc requested changes Apr 29, 2024
View reviewed changes
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 •
edited

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
Copy link
Member

@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
View reviewed changes
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
Copy link
Member

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
@github-actions GitHub Actions
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

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 7, 2024 16:31
@github-actions GitHub Actions
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 •
edited

@jasonhenriquez can we get this reviewed now?

jasonhenriquez
jasonhenriquez requested changes May 7, 2024
View reviewed changes
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
// Reset search filters if searching another query
if (this.$route.params.query && this.$route.params.query !== query) {
this.$store.commit('setSearchSettingsToDefault')
this.searchFilterValueChanged = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.searchFilterValueChanged = false
this.$store.commit('setSearchFilterValueChanged', false)

issue (blocking): To update values in the Vuex store, use mutations.

src/renderer/components/ft-search-filters/ft-search-filters.vue
@@ -44,6 +47,7 @@
:title="$t('Search Filters.Duration.Duration')"
:labels="durationLabels"
:values="durationValues"
:selected="searchSettings.duration"
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (blocking): You should be able to remove the ft-radio-button and ft-search-filters changes altogether once you have implemented the other suggested change.

@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 •
edited

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonhenriquez jasonhenriquez jasonhenriquez requested changes

@PikachuEXE PikachuEXE PikachuEXE left review comments

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
PR: changes requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Active filter must reset after i search for something else
4 participants
@ankitchauhan-aka @PikachuEXE @efb4f5ff-1298-471a-8973-3d47447115dc @jasonhenriquez