-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: keyboard toolbar #346
Conversation
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Basically it works, but the current codebase is far from being ideal (CI explicitly shows that 😁), but main algorithm for view traversal should be working 👀 @VladyslavMartynov10 may I ask you to try to integrate it into your project and give a feedback of which features are missing/what could be improved? @IvanIhnatsiuk tagging you since you also wanted to check an implementation and do some experiments (also hope for your help with TouchableNativeFeedback on Android) |
Hey @kirillzyusko ! Thanks for updates, I'll try to use it tomorrow. |
Hello everyone! In my opinion, this first version of the KeyboardToolBar is functioning quite well and covers most of the basic scenarios. Here's a list of potential improvements:
I've made a brief video demonstrating how it operates on a Samsung device with the One Ui system. Tested both in 60/120 FPS mode. Here are results: 60 FPS: FILE.2024-01-31.22.25.18.mp4120FPS: Test_120.mov |
Hello @VladyslavMartynov10 👋
I think it's already doable via
Agree, I think we need to:
I think the amount of input doesn't matter, though from performance perspective it would be definetely a good point!
Need to check that! Regarding FPS count - I think it doesn't change a lot between 60 and 120 FPS. Am I right? |
Hey @kirillzyusko ! In comparison to my initial JS version which takes about average 7-12 PFS lost, native implementation definitely takes the lead 😁. |
84044fd
to
302a35d
Compare
📊 Package size report
|
Ah, glad to know, that native implementation works faster 😎 I'll take into consideration all feedbacks that you provided and when new version will be avaialble for testing I'll ask you once again to test it 😊 |
fe20544
to
109bd01
Compare
…e` folder (Android) (#353) ## 📜 Description Move all interactive keyboard code to dedicated `interactive` folder/package in kotlin/Android. ## 💡 Motivation and Context Ideally in root android folder I'd like to keep a single `package` file and all other code should be placed in folders. Initially I created generic purpose folders, like `events`/`listeners`/`views` etc. However sometimes it may be hard to create generic purpose folders for specific files. Theoretically I could create folders `controllers`/`providers`, but they would contain only single files. So I took an inspiration from #346 PR, where I created `traversal` folder for keeping functionality related to view traversal/keyboard toolbar. And in this PR I moved all interactive-keyboard related code into `interactive` folder - it keeps file-system clean 😎 ## 📢 Changelog <!-- High level overview of important changes --> <!-- For example: fixed status bar manipulation; added new types declarations; --> <!-- If your changes don't affect one of platform/language below - then remove this platform/language --> ### CI - free up disk for `e2e` tests (Android); ### Android - added `interactive` folder; - move `interpolators` folder to `interactive`; - moved `InteractiveKeyboardProvider` and `KeyboardAnimationController` to `interactive` folder. ## 🤔 How Has This Been Tested? Tested on CI. ## 📸 Screenshots (if appropriate): |Before|After| |-------|-----| |<img width="279" alt="image" src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/e441f44b-865f-4fb7-97c1-d303ee951bbb">|<img width="351" alt="image" src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/0abc31f3-6a38-4a6e-aa44-4f9dbd95c741">| ## 📝 Checklist - [x] CI successfully passed - [x] I added new mocks and corresponding unit-tests if library API was changed
109bd01
to
3af93a7
Compare
@VladyslavMartynov10 @IvanIhnatsiuk I fixed all the comments (and overall current version looks much more stable than a previous version). Would you mind to test it and give your feedback? 👀 |
Looks stable 😀 Any bugs will be handled in separate branch 👀 |
📜 Description
Added keyboard toolbar component.
💡 Motivation and Context
The feature was requested in #303
Before the implementation I decided to make a research to see how similar tasks are implemented on other platforms/framework. I went through:
The downside of
keyboard_actions
from Flutter is the fact that it requires a lot of hand engineering (settingrefs
to each input and the passing this array to the keyboard). And we need to do that for each screen.However
IQKeyboardManager
handles it automatically via view traversal. I thought that a similar algorithm can be implemented in a react-native, because all JS views are shown in a native view hierarchy as corresponding native views.The
IQKeyboardManager
offers a lot of customization which is not needed inreact-native
ecosystem (like specifying array ofUIVIewController
s whereKeyboardToolbar
should be hidden - it should be done via conditional rendering in RN world). On the other side - solution fromIQKeyboardManager
is not very customizable inreact-native
ecosystem (we can not specify rendering for custom elements).So I took inspiration from all frameworks and decided to create a new approach that will ideally fit
react-native
projects - it'll not require a lot of hand engineering and will offer a great flexibility and customization. The ideal goal is to have the API as on web (from simplicity perspective) and to be a very cusomizable.IQKeyboardManager
handles a lot of cases, but I decided to start from something basic and then enhance the API. So the core API is a new method forKeyboardController
-setFocusTo("next" | "prev")
. It'll go to the next/prev field from currently focused input. For that I implemented code that goes through view hierarchy on both platforms.Later I started to analyze further specifications and decided to add
setFocusTo("current")
API that will bring a keyboard back (set focus to last focused input).Based on this API I created a precise replica of
KeyboardToolbar
(took a reference fromweb
on iOS) and tried to create something that looks similar on Android (overall matches iOS design but uses colors that matches system UI on Android).Remaining thing was how to tell JS when next/prev buttons available/active. I had few options on which way to use for dispatching:
eventEmitter
(which I selected in the end);KeyboardControllerView
(we would need to add data in context, and it's kind of strange to have a lot of unrelated data/methods in context, we start to break SOLID principles here);ToolbarView
(it could help to propagate events synchronously via reanimated-event-bus, but we also have to use conditional rendering/specify other props, so I thought it would be more convenient to use plain state - this component should be highly customizable and setting props viauseAnimatedProps
may be not a trivial thing for devs who just came to RN world);I decided to use first approach because it's very simple and it looks like it's good enough (in future it can be changed, I didn't make the API for events propagation public yet).
I also had a choice of which data structures to use:
vs
I selected a second one, because in my understanding it may give a better flexibility (though for this approach we have to gather all inputs on a screen which is more resource consuming operation than finding prev and next
TextInput
- again now it works fine so let's keep it, but in future internal events shape can be changed (for example if we encounter performance problems) - these events not part of public API. In public API we just propagatedisabled
property - the way how it's calculated it's totally up to library).Closes #303 facebook/react-native#641
📢 Changelog
Docs
KeyboardToolbar
API;KeyboardToolbar
usage;JS
KeyboardToolbar
component;setFocusTo
method (specs, TS declarations);interpolate
rather than an assigned shared value because it seems like setting happens asynchronously and initial render may be incorrect (if you addconsole.log({ a: interpolation, b: lastInterpolation.value });
you will see next output{"a": 42, "b": 0}, {"a": 42, "b": 0}, {"a": 42, "b": 42}
);iOS
FocusedInputHolder
class (holds a reference to last focused input);ViewHierarchyNavigator
which travers view hierarchy back and forth;setFocusTo
method toKeyboardController
;Android
robolectric
) forViewHierarchyNavigator
class;FocusedInputHolder
class (holds a reference to last focused input);ViewHierarchyNavigator
which travers view hierarchy back and forth;setFocusTo
method toKeyboardController
;E2E
KeyboardToolbar
functionality;expectElementBitmapsToBeEqual
anddoActionNTimes
;CI
🤔 How Has This Been Tested?
Tested manually on:
📸 Screenshots (if appropriate):
Screenshots
Videos
telegram-cloud-document-2-5363837978546293906.mp4
📝 Checklist