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

Update search functionality #39

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Update search functionality #39

merged 2 commits into from
Jan 26, 2023

Conversation

SamuZad
Copy link

@SamuZad SamuZad commented Jan 12, 2023

Update the search functionality with the following:

  1. Modified the event listener so that it only starts running when Enter is pressed. This prevents the aggressive reload every time the searchString changes.
  2. Moved the "No matching key" messages out of the for loop, so they don't get sent every time a secret is iterated on. Instead, a message will be sent at the end of the search.
  3. Split the "No matching key" messages, so they better indicate whether it was a search or "default page load behaviour" that yielded no results.
  4. Modified search behaviour so that it can search for substrings. Ie if the secret is "bbc.com", and we search for "bbc.co", it will be found as a match during manual search.
  5. After the search string is set to empty and Enter is pressed, the default behaviour will take place

popup.js Show resolved Hide resolved
@mulbc
Copy link
Owner

mulbc commented Jan 14, 2023

@SamuZad & @Gerard-CK
With the sudden influx of good PRs i do have to wonder... Are you working for the same company?

@SamuZad
Copy link
Author

SamuZad commented Jan 14, 2023

@SamuZad & @Gerard-CK With the sudden influx of good PRs i do have to wonder... Are you working for the same company?

To my knowledge.. we have nothing to do with each other 😅

@Gerard-CK
Copy link

@SamuZad & @Gerard-CK With the sudden influx of good PRs i do have to wonder... Are you working for the same company?

To my knowledge.. we have nothing to do with each other 😅

I concur. I think it's a case of great minds think alike. :-)

@SamuZad
Copy link
Author

SamuZad commented Jan 15, 2023

@mulbc the debouncer will be handled as a separate PR. Could we get a merge, if you are happy with this?

@mulbc
Copy link
Owner

mulbc commented Jan 16, 2023

There are a few more changes in here than I can immediately overlook. Thus I would like to do some more verification before I merge this, which will take me a few days.
Thanks for your patience and great work in assembling all the PRs

Copy link
Owner

@mulbc mulbc left a comment

Choose a reason for hiding this comment

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

lgtm

@mulbc mulbc merged commit 8764a3f into mulbc:master Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants