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

feat: Add onClearIconPress to SearchBar #3679

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Conversation

n0tl3ss
Copy link
Contributor

@n0tl3ss n0tl3ss commented Feb 15, 2023

Summary

This PR adds the onClearIconPress prop to the Searchbar component. If onClearIconPress is defined when pressing close button on search the default behaviour will be executed and after that users' hook will be executed. This for an example allows Keyboard.dismiss() to be called from hook.

Test plan

There is no visually changes. Adding
onClearIconPress={() => /* delete query text (default behaviour) and dismiss keyboard */ Keyboard.dismiss()} to SearchBar should dismiss keyboard after clearing text.

This feature was requested in #1970

@n0tl3ss n0tl3ss force-pushed the main branch 2 times, most recently from 1f3345d to d864c27 Compare February 15, 2023 12:20
@callstack-bot
Copy link

callstack-bot commented Feb 15, 2023

Hey @n0tl3ss, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@n0tl3ss n0tl3ss force-pushed the main branch 3 times, most recently from af701c0 to fab09fe Compare February 15, 2023 22:06
root.current?.clear();
rest.onChangeText?.('');
onClearIconPress?.(e);
Copy link
Member

Choose a reason for hiding this comment

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

In order to accept the PR please add an unit test confirming that onClearIconPress is called when clear button is pressed

@n0tl3ss
Copy link
Contributor Author

n0tl3ss commented Mar 6, 2023

Hi @lukewalczak thanks for your reply. Sorry for late response I was on vacation. I think now everything is ready.

@lukewalczak
Copy link
Member

Hey @n0tl3ss, thanks for PR adjustments. Please correct the commit message in order to merge it (according to the contribution guide).

@n0tl3ss
Copy link
Contributor Author

n0tl3ss commented Mar 6, 2023

Hi, @lukewalczak. Is commit message now ok? :)

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

Thanks @n0tl3ss for addressing the feedback and adding a new functionality!

@lukewalczak
Copy link
Member

The commit message is still not correct, it should be feat: ... (missing colon)

@n0tl3ss n0tl3ss changed the title feat Add onClearIconPress to SearchBar feat: Add onClearIconPress to SearchBar Mar 6, 2023
@n0tl3ss
Copy link
Contributor Author

n0tl3ss commented Mar 6, 2023

@lukewalczak I think now commit and title of PR is fixed

@lukewalczak lukewalczak merged commit 9125545 into callstack:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants