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.

Sign up for GitHub

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

Jump to bottom

Add API for imperative actions on RNSSearchBar #1523

Closed
wants to merge 1 commit into from
Closed

Add API for imperative actions on RNSSearchBar #1523

wants to merge 1 commit into from

Conversation

egegunes
Copy link
Contributor

@egegunes egegunes commented Jul 9, 2022 •
edited
Loading

Description

This changes allow users to manipulate the RNSSearchBar. For use cases please see discussions/1323.

Changes

  • Exported 5 new methods from RNSSearchBarManager in ios/RNSSearchBar.mm.
    1. focus
    2. blur
    3. toggleCancelButton
    4. clearText
  • Also added the same methods to SearchBar in src/index.native.tsx to call native ones
  • Added ref key to SearchBarProps in src/types.tsx

Test code and steps to reproduce

Added buttons to Example app to test each action:

  1. Focus
  2. Blur
  3. ToggleCancelButton
  4. ClearText

Checklist

@kkafar kkafar self-assigned this Jul 25, 2022
@egegunes
Copy link
Contributor Author

@kkafar I fixed the conflict and updated the docs, please let me know if it needs any improvements. We're using this in our internal builds for some time and it looks good so far.

@kkafar
Copy link
Member

kkafar commented Aug 17, 2022

Hi @egegunes!
Thank you for your contribution 🎉 It looks good, although I believe these changes may have some issues on Fabric, but it needs to be tested.

I'll do it & let you know in couple of days.

@kkafar kkafar mentioned this pull request Oct 11, 2022
Merged
15 tasks
@kkafar
Copy link
Member

kkafar commented Oct 11, 2022

Sorry for the delay, I'm back on it.

Your PR looks good and would be ready to merge, but I indeed encountered some issues while implementing the code for Fabric (can't get to codegened native commands w/o registering the RNSSearchBar component twice, you can reproduce on #1610) so I won't merge it for now, as I want to have keep the API identical for both architectures. However thank you for your contribution! I'll have it merged as soon as I tackle with Fabric implementation.

@kkafar kkafar mentioned this pull request Nov 17, 2022
Closed
@felixaa
Copy link

felixaa commented Dec 2, 2022 •
edited
Loading

This is awesome @egegunes. I'd love to contribute but my knowledge in objc is quite limited. Could you have a look at this discussion and help me assess if this is a low-hanging fruit that could be included with this api changes? I'd love to help out!! Thanks!

#1617

@felixaa
Copy link

felixaa commented Dec 13, 2022

@egegunes I've created a PR on your fork for supporting inserting value into the search bar.
Could you have a look? <3

@ha3
Copy link

ha3 commented Jan 25, 2023

@kkafar since react native 0.71 has landed; is there anything preventing the Fabric issue from being solved? I would love this PR to get merged and would like to offer help if needed.

@kkafar
Copy link
Member

kkafar commented Feb 8, 2023

@kkafar since react native 0.71 has landed; is there anything preventing the Fabric issue from being solved? I would love this PR to get merged and would like to offer help if needed.

@ha3, no, I'm back on it. Thank you for offer!. I'll try to release it with next minor version.

@kkafar
Copy link
Member

kkafar commented Feb 9, 2023

Thank you for contribution!
I'll close this PR, as I've cherry-picked these changes to #1610 and will continue development there.
Right now I have working unified Fabric and Paper implementation on iOS. I haven't played with Android yet.

@kkafar kkafar closed this Feb 9, 2023
kkafar added a commit that referenced this pull request Feb 24, 2023
@kkafar @egegunes
feat: add imperative API for search bar (#1610)
4cbd9cb 
## Description

This PR is a follow up to #1523 adding code for new React Native
architecture.

Status:

* [x] iOS Paper
* [x] iOS Fabric
* [x] Android Paper
* [x] Android Fabric

TODO:

* [x] Check when native commands (`dispatchCommand`) mechanism was
introduced and back to what RN version is it compatible with Paper
(seems to be working properly at least back to rn 0.68, need to check
earlier versions)
* [x] Implement Android
* [x] Refine type declarations in `types.tsx`


**Note**: while working on android implementation I've found few bugs

<details>
<summary>Scrollview does not respect header (Paper)</summary>

<img width="745" alt="image"
src="https://user-images.githubusercontent.com/50801299/218068165-b0ae7154-5444-49ae-8bf4-d3af6a9176c5.png">

</details>


## Changes

Added following methods:

* `focus()`
* `blur()`
* `clearText()`
* `toggleCancelButton(show: boolean)`

For usage please see `Test1097` in `TestsExample`.

## Test code and steps to reproduce

Added `Test1097` in `FabricTestExample` && `TestsExample`

## Checklist

- [x] Included code example that can be used to test this change
- [x] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Ege Güneş <ege.gunes@percona.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kkafar kkafar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@egegunes @kkafar @felixaa @ha3