Add Debounce to filter selection #2104

anton202 · 2023-01-18T12:29:15Z

Fixes

Description

as described in the issue by @sarayourfriend, currently when selecting several filters one after the other
a query will be excuted for each filter selection.
so, as suggested in the issue i added a debounce of 500ms to filter selection (basically switched the watch function to watchDebounced )

Staging

staging-filter.mp4

This PR

staging-filter.mp4

Testing Instructions

follow the steps in the video :)

Checklist

My pull request has a descriptive title (not a vague title like
Update index.md).
My pull request targets the default branch of the repository (main) or
a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible
errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

anton202 · 2023-01-19T09:52:24Z

Hi, @zackkrida
i`m having a wierd thing with e2e/mobile-menu-old.spec.ts test.
currently it fails, it fails also when i run it locally.
but, if i set the new_header:{status:{staging:disabled}} at features-flags.json
the test does pass.
maybe its something related to enableOldHeader function at the e2e/mobile-menu-old.spec.ts test ?

i see that it also happened here #2090

sarayourfriend · 2023-01-19T22:44:19Z

@obulat would probably know best how to debug this issue quickly 🤞

obulat · 2023-01-20T06:51:14Z

@anton202, I think you opened this PR before the new_header flag was set to be on by default in the staging environment. So, in your branch, new_header.status.staging in features-flags.json is set to switchable, and the defaultValue is off, but in main it was changed to on. You will probably need to rebase onto (updated) main branch to fix the problems.

If there are still problems with the e2e remaining, I think we will also need to change the enableOldHeader function, just like you mentioned.

I tried rebasing your branch onto main, and changing the functions to what's listed below, and didn't get any errors in e2e tests locally.

--- a/test/playwright/utils/navigation.ts
+++ b/test/playwright/utils/navigation.ts
@@ -464,19 +464,12 @@ export const enableNewHeader = async (page: Page) => {
 
 export const enableOldHeader = async (page: Page) => {
   // Add the new_header cookie
-  await page.context().addCookies([
-    {
-      name: "features",
-      value: "%7B%22new_header%22%3A%22off%22%7D",
-      domain: "localhost",
-      path: "/",
-    },
-  ])
+  await setCookies(page.context(), { features: { new_header: "off" } })
 }
 
 export const setCookies = async (
   context: BrowserContext,
-  cookies: Record<string, string | boolean | string[]>
+  cookies: Record<string, string | boolean | string[] | Record<string, string>>
 ) => {
   await context.addCookies(
     Object.entries(cookies).map(([name, value]) => ({

obulat

Thank you for your fix, @anton202, it works perfectly! I've added notes about the tests, and I'll approve after they are fixed.

anton202 · 2023-01-20T07:18:17Z

@obulat Thank you for the help. i`ll adress that on sunday :)

test/playwright/e2e/search-navigation-old.spec.ts

test/playwright/e2e/search-navigation.spec.ts

src/components/VFilters/VSearchGridFilter.vue

anton202 · 2023-01-22T14:34:55Z

@obulat i`ve tried your suggestion but the test still didnt pass :(
i did noticed though that on this branch and on the main branch new_header.status.staging is set to enabled
i changed it to switchable and now the test passing.
btw, right now we can`t toggle betwen the new and old header on staging. i think beacuse new_header.status.staging is set to enabled

anton202 · 2023-01-24T15:34:39Z

weird. all the tests pass when i run them locally

sarayourfriend · 2023-01-24T23:23:34Z

@anton202 Sorry for the delay in responses on this PR, folks have been travelling the past week and doing work offline. Most will be back later in the week and someone will be able to help debug more closely. We appreciate the patience 🙏

anton202 · 2023-01-26T08:21:41Z

sure, no problem :)

Co-authored-by: Zack Krida <zackkrida@pm.me>

openverse-bot · 2023-02-07T00:00:06Z

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was updated 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@anton202, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

dhruvkb

LGTM and it works. But I have a question about the wait numbers. A code comment // explaining them would be good.

dhruvkb · 2023-02-08T04:32:17Z

src/components/VFilters/VSearchGridFilter.vue

      () => searchStore.searchQueryParams,
      (newQuery: ApiQueryParams, oldQuery: ApiQueryParams) => {
        if (!areQueriesEqual(newQuery, oldQuery)) {
          router.push(searchStore.getSearchPath())
        }
-      }
+      },
+      { debounce: 800, maxWait: 5000 }


What is the reasoning behind these values?

Ah I see @zackkrida wanted to raise it from 500. 800 seemed a very specific, very arbitrary choice to start with.

anton202 requested a review from a team as a code owner January 18, 2023 12:29

anton202 requested review from zackkrida and dhruvkb January 18, 2023 12:29

openverse-bot added ✨ goal: improvement Improvement to an existing user-facing feature 🕹 aspect: interface Concerns end-users' experience with the software 🟨 priority: medium Not blocking but should be addressed soon labels Jan 18, 2023

obulat suggested changes Jan 20, 2023

View reviewed changes

zackkrida reviewed Jan 21, 2023

View reviewed changes

test/playwright/e2e/search-navigation-old.spec.ts Outdated Show resolved Hide resolved

zackkrida reviewed Jan 21, 2023

View reviewed changes

test/playwright/e2e/search-navigation.spec.ts Outdated Show resolved Hide resolved

zackkrida reviewed Jan 22, 2023

View reviewed changes

src/components/VFilters/VSearchGridFilter.vue Outdated Show resolved Hide resolved

anton202 and others added 6 commits January 31, 2023 09:32

changed watch to watchDebounced

5177681

test update

48ed98a

changed debonce value

0723f44

Updatecomment

bd83f5b

Co-authored-by: Zack Krida <zackkrida@pm.me>

Update comment

30e9a77

Co-authored-by: Zack Krida <zackkrida@pm.me>

update comment

c2c1254

obulat approved these changes Jan 31, 2023

View reviewed changes

dhruvkb approved these changes Feb 8, 2023

View reviewed changes

obulat merged commit d2926b7 into WordPress:main Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Debounce to filter selection #2104

Add Debounce to filter selection #2104

anton202 commented Jan 18, 2023

anton202 commented Jan 19, 2023 •

edited

Loading

sarayourfriend commented Jan 19, 2023

obulat commented Jan 20, 2023

obulat left a comment

anton202 commented Jan 20, 2023

anton202 commented Jan 22, 2023

anton202 commented Jan 24, 2023

sarayourfriend commented Jan 24, 2023 •

edited

Loading

anton202 commented Jan 26, 2023

openverse-bot commented Feb 7, 2023

dhruvkb left a comment

dhruvkb Feb 8, 2023

dhruvkb Feb 8, 2023

Add Debounce to filter selection #2104

Add Debounce to filter selection #2104

Conversation

anton202 commented Jan 18, 2023

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

anton202 commented Jan 19, 2023 • edited Loading

sarayourfriend commented Jan 19, 2023

obulat commented Jan 20, 2023

obulat left a comment

Choose a reason for hiding this comment

anton202 commented Jan 20, 2023

anton202 commented Jan 22, 2023

anton202 commented Jan 24, 2023

sarayourfriend commented Jan 24, 2023 • edited Loading

anton202 commented Jan 26, 2023

openverse-bot commented Feb 7, 2023

Footnotes

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Feb 8, 2023

Choose a reason for hiding this comment

dhruvkb Feb 8, 2023

Choose a reason for hiding this comment

anton202 commented Jan 19, 2023 •

edited

Loading

sarayourfriend commented Jan 24, 2023 •

edited

Loading