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

Allow easily clearing search box #1026

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Allow easily clearing search box #1026

merged 5 commits into from
Feb 10, 2021

Conversation

samuelT2
Copy link
Contributor

Fix related to "add X to clear search bar", issue #92

  • Clear search bar with ESC or X (X only works in Chrome)
  • Hide search bar when conversation or contact is clicked

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2021

CLA assistant check
All committers have signed the CLA.

@samuelT2
Copy link
Contributor Author

CLA is signed, not sure why it's still showing as not signed.

@samuelT2
Copy link
Contributor Author

CLA shows as not signed because my e-mail address is not linked to my commit (now it is, but I cannot modify existing commits).

@dbrgn
Copy link
Contributor

dbrgn commented Jan 19, 2021

CLA shows as not signed because my e-mail address is not linked to my commit (now it is, but I cannot modify existing commits).

You can easily fix this:

git config user.email [email protected]
git commit --amend --reset-author
git push --force

- Clear search bar with ESC or X (only Chrome)
- Hide search bar when conversation or contact is clicked
@samuelT2
Copy link
Contributor Author

Very cool, did solve it, thanks!

Copy link
Contributor

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Thanks! Some notes below.

samuelT2 and others added 2 commits January 19, 2021 16:47
Noted for future commits.

Co-authored-by: Danilo Bargen <[email protected]>
Thanks, didn't check that.

Co-authored-by: Danilo Bargen <[email protected]>
@samuelT2 samuelT2 requested a review from dbrgn January 19, 2021 15:58
@samuelT2
Copy link
Contributor Author

Requested changes are done, anything else needed from my side? Sorry that I ask, my very first pull request...

@dbrgn
Copy link
Contributor

dbrgn commented Jan 21, 2021

Thanks for applying the fixes!

Requested changes are done, anything else needed from my side? Sorry that I ask, my very first pull request...

No, you did everything right. We're currently juggling with different tasks and priorities (especially now that a lot new users joined), so it may take a few days until we can review a submission!

I tested your changes, they seem to work nicely in Chromium! I like that the search box disappears when clicking on a conversation, that makes sense. However, in Firefox the ESC key does not seem to trigger the keyUp event 🤔 Maybe you can test as well? It's possible that Firefox does not consider all key types for the keyUp event.

I also had another thought: Right now ESC clears the input field if it contains a search. Maybe it could also hide the search box if it's already empty? This way, a search could be aborted by pressing ESC twice.

@samuelT2
Copy link
Contributor Author

Sure - no hurry!

I tested your changes, they seem to work nicely in Chromium! I like that the search box disappears when clicking on a conversation, that makes sense. However, in Firefox the ESC key does not seem to trigger the keyUp event 🤔 Maybe you can test as well? It's possible that Firefox does not consider all key types for the keyUp event.

With which Firefox version are you testing currently? That ESC event works fine me with, it clears the search field in Chrome and Firefox 84.0.2 (64-Bit).

I also had another thought: Right now ESC clears the input field if it contains a search. Maybe it could also hide the search box if it's already empty? This way, a search could be aborted by pressing ESC twice.

This one is nice, and not much of work. Will do it and push it again.

@dbrgn
Copy link
Contributor

dbrgn commented Jan 21, 2021

With which Firefox version are you testing currently? That ESC event works fine me with, it clears the search field in Chrome and Firefox 84.0.2 (64-Bit).

Hm, that's strange, I'm also on 84.0.2 and for me the KeyUp listener is never even called. I'll wait for your update, and will then test again to find out what the reason is.

If searchText is filled and ESC gets pressed --> Empty SearchBar
If searchText is empty and ESC gets pressed --> Toggle SearchBar off
Copy link
Contributor

@threema-danilo threema-danilo left a comment

Choose a reason for hiding this comment

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

One last change request, then this is ready to merge.

@threema-danilo threema-danilo changed the title add X to clear search bar #92 Allow easily clearing search box Feb 9, 2021
Chrome: Avoiding emptying and closing search bar at the same time
Copy link
Contributor

@threema-danilo threema-danilo left a comment

Choose a reason for hiding this comment

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

Thanks @samuelT2!

@threema-danilo threema-danilo merged commit 40b432a into threema-ch:master Feb 10, 2021
@samuelT2 samuelT2 deleted the add-X-to-clear-search-bar branch February 10, 2021 11:17
@threema-danilo
Copy link
Contributor

Deployed to https://web-beta.threema.ch/, testing welcome. If no major bugs in that release are discovered in the next 2-3 days, a public release will be made.

@threema-danilo
Copy link
Contributor

threema-danilo commented Mar 4, 2021

Unfortunately even small features can have undesired side effects...

It seems that when searching through a lot of conversations (I have over 230 conversations), after clicking on a conversation, the search clears almost instantly, but it sometimes takes multiple seconds until the conversation has opened.

This has a lot to do with the inefficient architecture of AngularJS, where every UI update can trigger a lot of processing code, which in turn may cause yet another UI update.

For now, we might need to disable the clearing/hiding of the search when clicking on a search result. (Note: Clearing with ESC or by clicking on the X mark would remain.)

@samuelT2
Copy link
Contributor Author

samuelT2 commented Mar 4, 2021

Ouch, well - didn't test it with 230+ conversations :). Shall I revert that change or will you do it?

@threema-danilo
Copy link
Contributor

See #1042

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

Successfully merging this pull request may close these issues.

4 participants