Simplified search input & fixed chinese character input #3037

AWolf81 · 2019-05-25T19:14:40Z

Description

Reduced complexity in seach input string handling & removed some states
Tested chinese character search entry
Fixed focus issue that occured if your doing fast typing and backspace delete. That caused the editor area to be focused instead of the search input.

There are mutliple open PRs that are related to search input:

Fixes search input dropping key input #2942 - dropping key - I'll check if that is still happening here. I couldn't reproduce it here.
Chinese, Japanese, Korean are now available on search box. (fix for #2885) #2919 - also adds support for chinese characters by modifing handleSearchChange. I'm doing the same but with less code if it's OK to remove the isIME checking
Cannot search by Chinese #2433 - support of chinese chars by adding a compositionend eventl istener with a new state isIMEdone that will be set with compositionend - I think that's not really needed as directly handling in change handler should work.
Fix the search note functionality #2676 - Input only applied on backspace - I haven't noticed that here. It would be good to check the mentioned keyboards here too.

We should check if all of them are fixed with this PR.

Issue fixed

#2925, #3030, #2843

Type of changes

🔘 Bug fix (Change that fixed an issue)
⚪ Breaking change (Change that can cause existing functionality to change)
⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
⚪ Feature (Change that adds new functionality)
⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

🔘 My code follows the project code style
⚪ I have written test for my code and it has been tested
🔘 All existing tests have been passed
⚪ I have attached a screenshot/video to visualize my change if possible

Screenshot

Note just some chinese letters in there to show that you can now enter these characters

Todos:

Remove state variables isAlphabet, isIME, isConfirmTranslation (if it's OK to simplify this, see question below)
Add enter key handling to re-apply search term - handy if you navigate away and you want to use the same search term

Discussion

Is it OK to change the search input to not check the character range? I think that check is not really needed and just adds complexity. But please let me know if there is a reason I'm missing or a case that's not working with my change.
Please double check the chinese character entry. It should work but it's a bit difficult to test on a German laptop.
How can I test if my change to switchPreview === 'BLUR' || switchPreview === 'DBL_CLICK' is working? That change was needed to fix the focus loss but I'm not sure where it is used.
Fast typing issue: I'm not sure if it's fixed here because it could be related to notebook size. I have just a few notes and so searching is pretty fast. How can I debug this?

IssueHunt Summary

IssueHunt has been backed by the following sponsors. Become a sponsor

amedora · 2019-05-28T02:57:50Z

Unfortunately this changes break MS-IME behavior.
I can no longer input Japanese correctly... (tested on Win7/64)

AWolf81 · 2019-05-28T06:53:05Z

Oh, OK, can you please test if the following demo is working for you?
That seems to work & maybe we can use it or implement it directly. What do you think? Is it OK to use that component from npm?

It is using composition events to trigger the change event once the IME is done. I would use it as it's easier to read.

amedora · 2019-05-28T07:27:07Z

Oh, OK, can you please test if the following demo is working for you?

works nice 👍

I'm not sure it's ok or not to use that component, but looks good.

AWolf81 · 2019-05-28T18:20:05Z

@amedora can you please give it another try? I've added the composition input.
I have one point in the code where I'm not sure. I'll add an inline comment.

AWolf81 · 2019-05-28T18:22:58Z

browser/main/TopBar/index.js

@@ -165,7 +129,7 @@ class TopBar extends React.Component {
  }

  handleCodeInit () {
-    ee.emit('top:search', this.refs.searchInput.value)
+    ee.emit('top:search', this.refs.searchInput.value || '')


Why is this emit there? With out adding the || '' I've got an error from CodeEditor.js because it seems that the value is undefined here.
How can I test this?

LGTM. I didn't notice that it can be undefined.

AWolf81 · 2019-05-28T18:25:09Z

browser/main/TopBar/index.js

-      search: keyword
-    })
-    ee.emit('top:search', keyword)
+    const keyword = e.target.value


Here I wanted to use this.refs.searchInput.value but it wasn't working - maybe there is an issue with CInput and the ref but with event.target.value it is woking.
So that's OK and I just wanted to mention it.

amedora · 2019-05-29T02:34:45Z

LGTM. works like a charm 👍

# Conflicts: # browser/main/TopBar/index.js # yarn.lock

Rokt33r · 2019-06-03T04:47:30Z

@ZeroX-DG Could you test this too? I want to merge this soon. :)

ZeroX-DG · 2019-06-09T02:23:06Z

Works great for me!

simplified search input & fixed focus loss issue

25728e5

ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label May 25, 2019

AWolf81 added 2 commits May 28, 2019 20:16

use composition input

edc9d8b

Merge branch 'master' into fix-search-issue-unicode

9794149

AWolf81 commented May 28, 2019

View reviewed changes

AWolf81 added 3 commits May 29, 2019 07:42

Merge branch 'master' into fix-search-issue-unicode

25d055e

# Conflicts: # browser/main/TopBar/index.js # yarn.lock

fix focus loss by checking switchPrieview prop change

60e841e

apply search term on enter key

639bfbe

ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jun 9, 2019

Rokt33r merged commit b4251a7 into BoostIO:master Jun 28, 2019

Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Jun 28, 2019

Rokt33r added this to the v0.12.0 milestone Jun 28, 2019

AWolf81 deleted the fix-search-issue-unicode branch June 29, 2019 20:39

AWolf81 mentioned this pull request Jul 22, 2019

Cannot search by Chinese #2433

Closed

Rokt33r mentioned this pull request Jul 22, 2019

Fixes search input dropping key input #2942

Closed

This was referenced Jul 23, 2019

Search start only with delete last character #2015

Closed

Fix the search note functionality #2676

Closed

Chinese, Japanese, Korean are now available on search box. (fix for #2885) #2919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified search input & fixed chinese character input #3037

Simplified search input & fixed chinese character input #3037

AWolf81 commented May 25, 2019 •

edited by issuehunt-oss bot

Loading

amedora commented May 28, 2019

AWolf81 commented May 28, 2019

amedora commented May 28, 2019

AWolf81 commented May 28, 2019

AWolf81 May 28, 2019

Rokt33r Jun 3, 2019

AWolf81 May 28, 2019

amedora commented May 29, 2019

Rokt33r commented Jun 3, 2019

ZeroX-DG commented Jun 9, 2019

Simplified search input & fixed chinese character input #3037

Simplified search input & fixed chinese character input #3037

Conversation

AWolf81 commented May 25, 2019 • edited by issuehunt-oss bot Loading

Description

Issue fixed

Type of changes

Checklist:

Screenshot

Todos:

Discussion

amedora commented May 28, 2019

AWolf81 commented May 28, 2019

amedora commented May 28, 2019

AWolf81 commented May 28, 2019

AWolf81 May 28, 2019

Choose a reason for hiding this comment

Rokt33r Jun 3, 2019

Choose a reason for hiding this comment

AWolf81 May 28, 2019

Choose a reason for hiding this comment

amedora commented May 29, 2019

Rokt33r commented Jun 3, 2019

ZeroX-DG commented Jun 9, 2019

AWolf81 commented May 25, 2019 •

edited by issuehunt-oss bot

Loading