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(android): Add shouldClearOnSubmit-prop to search view #1639

Conversation

patricronge
Copy link

@patricronge patricronge commented Nov 16, 2022

Description

Adds an optional shouldClearOnSubmit-prop as boolean for android search view.
Clears the Search-input after after search button is pressed.

Screenshots / GIFs

ce15ab64d464b5da0a41ed92120d294f

Test code and steps to reproduce

Run the example project and go to Search bar. A settings switch for this prop has been added to the Android only-section.

Checklist

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

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

Hi @patricronge, great job! Thanks for the contribution and sorry for the huge delay of our response ❤️🙏 I just left some comments what is worth paying attention to, but the most important thing I believe is to change shouldClearOnSubmit to just clearOnSubmit.

@@ -46,6 +46,7 @@ const MainScreen = ({ navigation }: MainScreenProps): JSX.Element => {
const [autoCapitalize, setAutoCapitalize] =
useState<AutoCapitalize>('sentences');
const [inputType, setInputType] = useState<InputType>('text');
const [shouldClearOnSubmit, setShouldClearOnSubmit] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

I think clearOnSubmit would be a better name for the state and property overall.

Comment on lines +82 to +84
if (shouldClearOnSubmit != null) {
view.shouldClearOnSubmit = shouldClearOnSubmit
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this into

Suggested change
if (shouldClearOnSubmit != null) {
view.shouldClearOnSubmit = shouldClearOnSubmit
}
view.shouldClearOnSubmit = shouldClearOnSubmit ?: false

@@ -10,6 +10,7 @@ import androidx.appcompat.widget.SearchView
class SearchViewFormatter(var searchView: SearchView) {
private var mDefaultTextColor: Int? = null
private var mDefaultTintBackground: Drawable? = null
private var mDefaultText: String = ""
Copy link
Member

@tboba tboba Oct 3, 2023

Choose a reason for hiding this comment

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

Is default text needed here? I believe placeholder is being used as a default text to show when the prompt is empty - if this prop is not being served as a Search Bar prop, let's remove it.

@@ -64,4 +65,8 @@ class SearchViewFormatter(var searchView: SearchView) {
searchEditText?.hint = placeholder
}
}

fun clearText() {
searchEditText?.setText(mDefaultText)
Copy link
Member

Choose a reason for hiding this comment

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

According to the upper comment, let's change this to

Suggested change
searchEditText?.setText(mDefaultText)
searchEditText?.setText(null)

Copy link
Member

@tboba tboba Oct 3, 2023

Choose a reason for hiding this comment

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

As we don't hold the implementation of the Native Stack v5 anymore, I believe this change of the file is unnecessary.

Comment on lines +687 to +693
/**
* Clear the search input when search button is pressed.
*
* @plaform android
* @default false
*/
shouldClearOnSubmit?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

As I said, I'd suggest changing the name of this prop to clearOnSubmit.

@tboba
Copy link
Member

tboba commented Oct 5, 2023

Hi @patricronge, a small update from me - it looks like there's existing implementation of such behavior 😅

For clearing the search bar query, you can listen to the onSearchButtonPress and clear the text there with SearchBar commands.

  const searchBarRef = useRef<SearchBarCommands>(null);

// screenOptions or navigation#setOptions

          ref: searchBarRef,
          onSearchButtonPress: () => {
            searchBarRef.current.clearText();
          }

I'll close this PR for now, but if you'd like to change current search bar API don't hesitate to create new PR.
Cheers!

@tboba tboba closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants