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

Keyboard added, input is handled #130

Closed
wants to merge 12 commits into from
Closed

Keyboard added, input is handled #130

wants to merge 12 commits into from

Conversation

aahutsal
Copy link
Contributor

Basically it's partially finished for name TextInput, but I'd like to discuss keyboard flickering. According to this thread that won't be possible to fix for current React-Native version. Solution is to use custom controls in place of TextInput, but this would require more research and work. Please, lemme know.

@0xthreebody
Copy link
Collaborator

0xthreebody commented Jun 27, 2019

@agutsal Hey, Nice to see these. I will to try it. Really need a custom controls replacement TextInput.

@aahutsal
Copy link
Contributor Author

@jiangfuyao this is another big task. I'm assured every custom control will hit Keyboard.arise() or whatever is the method name without an option to control that. We should a) either find out some which won't b) fork some open source control, fix that and maintain it.

I'd prefer to finish this one first (attach what we have to password items, fix some UI stuff) then close that project then start new one.

@0xthreebody
Copy link
Collaborator

@agutsal Now in the early stages of the project, use the a) solution first.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jun 27, 2019

@jiangfuyao Check this: https://github.com/halilb/react-native-textinput-effects

Currently I'm checking how does they handle Keyboard.

@aahutsal
Copy link
Contributor Author

@jiangfuyao solution above won't work as they use TextItem internally. This seems to be better one: https://github.com/themodernjavascript/react-native-custom-keyboard-kit

Since it was not requested initially to replace controls, don't you mind adding 100 DAI to current project, @jiangfuyao ?

I'm working on solution 2 now, hopefully will get it done by the end of the day.

@aahutsal
Copy link
Contributor Author

@jiangfuyao #130 updated. Please try custom numeric keyboard for Name input. Let me know.

@aahutsal
Copy link
Contributor Author

react-native-custom-keyboard-kit seems to be pretty buggy also. Unmaintainable and hardly use Java inside. Will let you try, then reverting back to old implementation and waiting you to discuss, @jiangfuyao

@0xthreebody
Copy link
Collaborator

react-native-custom-keyboard-kit is not a good implementation.

@aahutsal
Copy link
Contributor Author

@jiangfuyao git push --force to previous one. I said - that's another big task to find the right approach/library. That was not planed from the beginning.

@aahutsal
Copy link
Contributor Author

Reverted. I'll finish what we have now. Just wasted 5 hours today on react-native-custom-keyboard-kit, damn.

@aahutsal
Copy link
Contributor Author

@jiangfuyao lemme try their demo

@aahutsal
Copy link
Contributor Author

@jiangfuyao this one works for me. Lemme add this implementation

@aahutsal
Copy link
Contributor Author

@jiangfuyao #130 updated. Looks good to me (added to first input only yet)

@aahutsal
Copy link
Contributor Author

@jiangfuyao thanks for that link btw ;)

@0xthreebody
Copy link
Collaborator

Now, full support for all inputs. Then I will check and merge. Thanks.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jun 28, 2019

Key component and both passwords are now custom also, @jiangfuyao
Check it.

@0xthreebody
Copy link
Collaborator

Note: other pages also have the input box.

@aahutsal
Copy link
Contributor Author

Noted. There are 49 more such entries. Since original project stated only password and private key inputs
image
I'd consider these 49 entries Phase II. Sounds fair?

@0xthreebody
Copy link
Collaborator

@agutsal The other pages only password need the safe keyboard.

@aahutsal
Copy link
Contributor Author

@jiangfuyao check this one

@0xthreebody 0xthreebody self-requested a review June 29, 2019 05:38
Copy link
Collaborator

@0xthreebody 0xthreebody left a comment

Choose a reason for hiding this comment

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

Tested good, but there have two small issue:

  • The original keyboard has focus function, the custom keyboard has no focus function:

深度录屏_选择区域_20190629135008

  • Occasionally, a double keyboard appears. When using a computer keyboard input, it appears.

深度截图_选择区域_20190629134806

Note: If this fixed, I will merging these changes.

@aahutsal
Copy link
Contributor Author

These both are related:
lyxia/react-native-yusha-customKeyboard#13

I can try using another repo for that component with that issue fixed, but we'll have system keyboard flickering again.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jul 1, 2019

@jiangfuyao Check recent commit (CreateAccount screen). It's still not perfect, keyboard is flickering, but system keyboard gets dismissed once shown. As I already said there's no perfect solution on that ReactNative version. We should come to some agreement.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jul 2, 2019

@jiangfuyao If you reviewed my recent commit I'd like to know your thoughts.

@aahutsal aahutsal changed the title [WIP] Keyboard added, input is handled Keyboard added, input is handled Jul 3, 2019
@0xthreebody
Copy link
Collaborator

@agutsal I will try it.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jul 4, 2019

@jiangfuyao thx

@0xthreebody
Copy link
Collaborator

The 2th issue is fixed, but 1th not. On the android, the custom keyboard has no focus function.

@aahutsal
Copy link
Contributor Author

aahutsal commented Jul 5, 2019

@jiangfuyao I've found that component. Hopefully it will do what you need. Will buzz you once I update the code.

@0xthreebody 0xthreebody added the pleasereview Pull request needs review. label Jul 10, 2019
@aahutsal
Copy link
Contributor Author

@jiangfuyao seems autofocus works now.

@0xthreebody
Copy link
Collaborator

@agutsal Testing...

@0xthreebody
Copy link
Collaborator

@agutsal Is nice, I will to merge this after resolved conflicts.

@aahutsal
Copy link
Contributor Author

@jiangfuyao from what I see there are conflicting files. Conflicts are caused Windows CRLF symbol in the end of each line. Since that's Windows-only EOL symbol, Git suggests not to commit it. This could be changed with help from here. However, if you want to keep CRLF EOL symbol, I'll re-configure my git to add it to each line and will try merging again.

Let me know.

@0xthreebody
Copy link
Collaborator

@agutsal Keep it, and I'll delete it later version, thanks!

@aahutsal
Copy link
Contributor Author

@jiangfuyao that's not correct decision. I've analyzed what have broken things and found, that your recent commit b4a3b0b (that' merged PR #135) did so.
PR #135 has 18k changed lines, not because it's super cool mega commit, but because its author(s) has changed CR to CRLF everywhere in codebase. I'm wondered how git pre-commit hook (that use husky that required lint npm script execution allowed so), perhaps husky was just temporarily switched off by committer, otherwise it would --fix all sources, replacing CRLF back to CR everywhere. Now, if I follow your commands we would have problems. I'd suggest to reset to 0d282ef (reverting #135 PR), push --force then fix #135 and merge it back. Then I can easily push my changes.

If not - everyone who is currently working on your code will have the same problems I have now.

@0xthreebody
Copy link
Collaborator

@agutsal I changed EOL from CRLF to LF, #137
this is your master branch to pull polkawallet-RN fix-eol branch, just for test. Is it ok now? Only can use CR?

@aahutsal
Copy link
Contributor Author

Trying. Will let you know shortly @jiangfuyao

@aahutsal
Copy link
Contributor Author

@jiangfuyao that was a nice talk. I'm closing that pull request, patched recent origin/fix-eol branch and created new PR

@aahutsal aahutsal closed this Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pleasereview Pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants