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

(Desktop) Fixes #4769: highlight existing text in global search field #4773

Merged
merged 2 commits into from
Apr 6, 2021
Merged

(Desktop) Fixes #4769: highlight existing text in global search field #4773

merged 2 commits into from
Apr 6, 2021

Conversation

coderrsid
Copy link
Contributor

@coderrsid coderrsid commented Mar 30, 2021

Fixes #4769

Problem :

When using the shortcut (default: CtrlCmd+Shift+F) or the menu item Edit > Search in all notes, the focus is set to the global search field. However, if there's already text in the field, the cursor is placed at the end and typing appends to the existing search term.

Solution:

This change should highlight the current text, so that it will be replaced by the new search term.

Unit Test

Not required as it's a selection of text if the focus on input element.

Manual Testing (after merging)

  1. Use shortcut (default: CtrlCmd+Shift+F) or the menu item Edit > Search in all notes, the focus is set to the global search field.
  2. If there's already text in the field, all the text in field is highlighted, and new characters will replace it.

Before

After

Screen-Recording-2021-03-30-at-3

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

LGTM

@tessus tessus added the desktop All desktop platforms label Mar 30, 2021
@laurent22
Copy link
Owner

Please read the guidelines before posting a pull request. You didn't provide test units, didn't explain why they are missing, didn't provide steps to manually test. Just because it's obvious to you doesn't mean you can just ignore the guidelines.

@tessus
Copy link
Collaborator

tessus commented Mar 30, 2021

He posted a video how it looks after. That is IMO the test case for this issue.

(Only the before case was taken from my original issue.)

@coderrsid
Copy link
Contributor Author

Please read the guidelines before posting a pull request. You didn't provide test units, didn't explain why they are missing, didn't provide steps to manually test. Just because it's obvious to you doesn't mean you can just ignore the guidelines.

Ohh sorry laurent, didn't mean to do that. Obviously, i forgot to mention about the unit tests. Although if you look at description PROBLEM, those are the steps to test problem and solution. Although, i'll add them as bulleted points as needed. Didn't mean to ignore any, fault's on my side. On it already.

@tobiasb
Copy link

tobiasb commented Apr 6, 2021

I'm just wondering, is there a plan to merge this anytime soon?

@coderrsid
Copy link
Contributor Author

I'm thinking that too. Not able to make any other fixes too.

@laurent22
Copy link
Owner

Yes that looks good, thanks for the pull request @coderrsid.

@laurent22 laurent22 merged commit 17792d9 into laurent22:dev Apr 6, 2021
@coderrsid coderrsid deleted the issue#4769 branch April 7, 2021 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

highlight existing text in global search field
5 participants