feat(Android): Expose configuration for showAsAction with collapse #3009
Conversation
| } | ||
|
|
||
| private fun updateToolbarMenu(menu: Menu) { | ||
| menu.clear() | ||
| if (shouldShowSearchBar()) { | ||
| locateSearchBarSubviewInConfig()?.let { |
There was a problem hiding this comment.
shouldShowSearchBar() was effectively true when config for type==SEARCH_BAR existed, so this refactor should not change the behaviour
There was a problem hiding this comment.
This is ok, thanks for the note.
| type StackNavigationProp = NavigationProp<StackRouteParamList>; | ||
|
|
||
| const SearchAlways = ({navigation}: StackNavigationProp) => { | ||
| return <ScrollView contentInsetAdjustmentBehavior='automatic'> |
There was a problem hiding this comment.
Let's use braces () - this will make code more maintainable, if some additional logic will be needed, also it'll stick to the pattern we use everywhere else.
Another thing - in general I prefer (and try to enforce) usage of regular functions over arrow functions, due to symbol hoisting. Therefore let's rewrite this a bit.
| @@ -733,6 +734,16 @@ export interface SearchBarProps { | |||
| * @platform ios | |||
| */ | |||
| tintColor?: ColorValue; | |||
| /** | |||
| * @platform android | |||
There was a problem hiding this comment.
This annotation should be placed in the end of the comment block. No technical reason, just to keep the convention.
There was a problem hiding this comment.
Why do we need this additional resource?
| actionView = searchView | ||
| setIcon(R.drawable.ic_action_search) |
There was a problem hiding this comment.
Why do we need to set this icon manually?
| val currentContext = context | ||
| if (searchView == null && currentContext != null) { | ||
| val newSearchView = CustomSearchView(currentContext, this) | ||
| searchView = newSearchView | ||
| onSearchViewCreate?.invoke(newSearchView) | ||
| } | ||
| menu.add("").apply { | ||
| setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS) | ||
| menu.add("Search").apply { |
| } | ||
|
|
||
| private fun updateToolbarMenu(menu: Menu) { | ||
| menu.clear() | ||
| if (shouldShowSearchBar()) { | ||
| locateSearchBarSubviewInConfig()?.let { |
There was a problem hiding this comment.
This is ok, thanks for the note.
| @@ -23,6 +23,8 @@ type SearchBarPlacement = 'automatic' | 'inline' | 'stacked'; | |||
|
|
|||
| type AutoCapitalizeType = 'none' | 'words' | 'sentences' | 'characters'; | |||
|
|
|||
| type SearchShowAsAction = 'default' | 'always' | 'collapse'; | |||
There was a problem hiding this comment.
Why do we need default case? Couldn't we just use always if the user does not set the value? Does it give us any advantage over direct approach?
|
Note Note for future us. On hold due to low prio + issues with existing code. There already exists code responsible for that interaction (preventing immediate pop when search bar is focused), however it stopped to work at some time. We need to figure out why it has stopped to work & fix it. |
Description
Resolves #2744
We have hardcoded
MenuItem.showAsActiontoSHOW_AS_ACTION_ALWAYSto have search icon always visible on toolbar. This setting also indirectly controls the behavior of Up button; OR-ing it with SHOW_AS_ACTION_COLLAPSE_ACTION_VIEW makes the Up button collapse the searchbar first, then pop the screen on the second click.Changes
Added
headerSearchBarActions.showAsActionoption for Screen with possible values:default,always,collapse, wheredefault === alwaysCurrently, no other SHOW_AS_ACTION_ enum is supported
show-as-action.mov
Test code and steps to reproduce
See
Test2744