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

fix: Change name of focus and blur events to searchFocus and searchBlur #2154

Merged
merged 9 commits into from
Jun 12, 2024
Merged

fix: Change name of focus and blur events to searchFocus and searchBlur #2154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6dacf11
Change name of focus and blur events to searchFocus and searchBlur
tboba May 21, 2024
299ce6a
Add types for native search bar
tboba May 22, 2024
2d17703
Merge branch 'main' into @tboba/fix-onfocus-onblur-errors-ios
tboba Jun 10, 2024
e26330e
Revert changes from first approach
tboba Jun 10, 2024
2ea8296
Fix issues with Searchbar.tsx
tboba Jun 10, 2024
922dbfd
Revert changes from types.tsx
tboba Jun 11, 2024
8bf91c8
Revert changes from types.tsx 2
tboba Jun 11, 2024
3ac0b65
Update main from react-navigation
tboba Jun 11, 2024
5e2bede
Revert renaming NativeCommands
tboba Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions android/src/main/java/com/swmansion/rnscreens/SearchBarManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,13 @@ class SearchBarManager : ViewGroupManager<SearchBarView>() {
override fun getExportedCustomDirectEventTypeConstants(): Map<String, Any>? {
return MapBuilder.of(
SearchBarBlurEvent.EVENT_NAME,
MapBuilder.of("registrationName", "onBlur"),
MapBuilder.of("registrationName", "onSearchBlur"),
SearchBarChangeTextEvent.EVENT_NAME,
MapBuilder.of("registrationName", "onChangeText"),
SearchBarCloseEvent.EVENT_NAME,
MapBuilder.of("registrationName", "onClose"),
SearchBarFocusEvent.EVENT_NAME,
MapBuilder.of("registrationName", "onFocus"),
MapBuilder.of("registrationName", "onSearchFocus"),
SearchBarOpenEvent.EVENT_NAME,
MapBuilder.of("registrationName", "onOpen"),
SearchBarSearchButtonPressEvent.EVENT_NAME,
Expand Down
2 changes: 1 addition & 1 deletion android/src/main/java/com/swmansion/rnscreens/events/SearchBarBlurEvent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ class SearchBarBlurEvent(surfaceId: Int, viewId: Int) : Event<SearchBarBlurEvent
override fun getEventData(): WritableMap? = Arguments.createMap()

companion object {
const val EVENT_NAME = "topBlur"
const val EVENT_NAME = "topSearchBlur"
}
}
2 changes: 1 addition & 1 deletion android/src/main/java/com/swmansion/rnscreens/events/SearchBarFocusEvent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ class SearchBarFocusEvent(surfaceId: Int, viewId: Int) : Event<SearchBarFocusEve
override fun getEventData(): WritableMap? = Arguments.createMap()

companion object {
const val EVENT_NAME = "topFocus"
const val EVENT_NAME = "topSearchFocus"
}
}
10 changes: 5 additions & 5 deletions ios/RNSSearchBar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@

#ifdef RCT_NEW_ARCH_ENABLED
#else
@property (nonatomic, copy) RCTBubblingEventBlock onChangeText;
@property (nonatomic, copy) RCTBubblingEventBlock onCancelButtonPress;
@property (nonatomic, copy) RCTBubblingEventBlock onSearchButtonPress;
@property (nonatomic, copy) RCTBubblingEventBlock onFocus;
@property (nonatomic, copy) RCTBubblingEventBlock onBlur;
@property (nonatomic, copy) RCTDirectEventBlock onChangeText;
@property (nonatomic, copy) RCTDirectEventBlock onCancelButtonPress;
@property (nonatomic, copy) RCTDirectEventBlock onSearchButtonPress;
@property (nonatomic, copy) RCTDirectEventBlock onSearchFocus;
@property (nonatomic, copy) RCTDirectEventBlock onSearchBlur;
kkafar marked this conversation as resolved.
Show resolved Hide resolved
#endif

@end
Expand Down
22 changes: 11 additions & 11 deletions ios/RNSSearchBar.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ - (void)emitOnFocusEvent
#ifdef RCT_NEW_ARCH_ENABLED
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSSearchBarEventEmitter>(_eventEmitter)
->onFocus(react::RNSSearchBarEventEmitter::OnFocus{});
->onSearchFocus(react::RNSSearchBarEventEmitter::OnSearchFocus{});
}
#else
if (self.onFocus) {
self.onFocus(@{});
if (self.onSearchFocus) {
self.onSearchFocus(@{});
}
#endif
}
Expand All @@ -81,11 +81,11 @@ - (void)emitOnBlurEvent
#ifdef RCT_NEW_ARCH_ENABLED
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSSearchBarEventEmitter>(_eventEmitter)
->onBlur(react::RNSSearchBarEventEmitter::OnBlur{});
->onSearchBlur(react::RNSSearchBarEventEmitter::OnSearchBlur{});
}
#else
if (self.onBlur) {
self.onBlur(@{});
if (self.onSearchBlur) {
self.onSearchBlur(@{});
}
#endif
}
Expand Down Expand Up @@ -414,11 +414,11 @@ - (UIView *)view
RCT_EXPORT_VIEW_PROPERTY(cancelButtonText, NSString)
RCT_EXPORT_VIEW_PROPERTY(placement, RNSSearchBarPlacement)

RCT_EXPORT_VIEW_PROPERTY(onChangeText, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onCancelButtonPress, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onSearchButtonPress, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onFocus, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onBlur, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onChangeText, RCTDirectEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onCancelButtonPress, RCTDirectEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onSearchButtonPress, RCTDirectEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onSearchFocus, RCTDirectEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onSearchBlur, RCTDirectEventBlock)

#ifndef RCT_NEW_ARCH_ENABLED

Expand Down
14 changes: 11 additions & 3 deletions src/components/SearchBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
isSearchBarAvailableForCurrentPlatform,
SearchBarCommands,
SearchBarProps,
RenamedSearchBarProps,
} from 'react-native-screens';
import { View } from 'react-native';

Expand All @@ -11,9 +12,9 @@ import SearchBarNativeComponent, {
Commands as SearchBarNativeCommands,
} from '../fabric/SearchBarNativeComponent';

export const NativeSearchBar: React.ComponentType<SearchBarProps> &
export const NativeSearchBar: React.ComponentType<RenamedSearchBarProps> &
typeof NativeSearchBarCommands =
SearchBarNativeComponent as unknown as React.ComponentType<SearchBarProps> &
SearchBarNativeComponent as unknown as React.ComponentType<RenamedSearchBarProps> &
SearchBarCommandsType;
export const NativeSearchBarCommands: SearchBarCommandsType =
SearchBarNativeCommands as SearchBarCommandsType;
Expand Down Expand Up @@ -76,7 +77,14 @@ function SearchBar(props: SearchBarProps, ref: React.Ref<SearchBarCommands>) {
return View as unknown as React.ReactNode;
}

return <NativeSearchBar {...props} ref={searchBarRef} />;
return (
<NativeSearchBar
onSearchFocus={props.onFocus}
onSearchBlur={props.onBlur}
{...props}
ref={searchBarRef}
/>
);
}

export default React.forwardRef<SearchBarCommands, SearchBarProps>(SearchBar);
4 changes: 2 additions & 2 deletions src/fabric/SearchBarNativeComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ type SearchBarPlacement = 'automatic' | 'inline' | 'stacked';
type AutoCapitalizeType = 'none' | 'words' | 'sentences' | 'characters';

interface NativeProps extends ViewProps {
onFocus?: DirectEventHandler<SearchBarEvent> | null;
onBlur?: DirectEventHandler<SearchBarEvent> | null;
onSearchFocus?: DirectEventHandler<SearchBarEvent> | null;
onSearchBlur?: DirectEventHandler<SearchBarEvent> | null;
onSearchButtonPress?: DirectEventHandler<SearchButtonPressedEvent> | null;
onCancelButtonPress?: DirectEventHandler<SearchBarEvent> | null;
onChangeText?: DirectEventHandler<ChangeTextEvent> | null;
Expand Down
21 changes: 21 additions & 0 deletions src/types.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,3 +713,24 @@ export interface SearchBarProps {
*/
shouldShowHintSearchIcon?: boolean;
}

/**
* Since the search bar is a component that is extended from View on iOS,
* we can't use onFocus and onBlur events directly there (as of the event naming conflicts).
* To omit any breaking changes, we're handling this type to rename onFocus and onBlur events
* to onSearchFocus and onSearchBlur inside the native component of the search bar.
*/
export type RenamedSearchBarProps = Rename<
SearchBarProps,
{ onFocus: 'onSearchFocus'; onBlur: 'onSearchBlur' }
>;

/**
* Helper type, used to rename certain keys in the interface, given from object.
*/
type Rename<
T,
R extends {
[K in keyof R]: K extends keyof T ? PropertyKey : 'Error: key not in T';
}
> = { [P in keyof T as P extends keyof R ? R[P] : P]: T[P] };
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the idea here (and like it btw.), however do we need this? If w/o this fix setting these options would crash application anyway I believe we can safely treat this as a required fix and not a breaking change, thus removing the need for such workaround.

This comment was marked as off-topic.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I require an assistance of @WoLewicki there, as I was speaking with him about this change 😁 but when I was wondering about it, I was thinking that it would also require us to introduce the breaking change inside the react-navigation, and also there will be time for introducing such breaking changes (very soon 🤭). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to change the user's API for sure so the SearchBarProps must stay the same but NativeSearchBar should take its props from the spec inside fabric directory so we are sure we are sending the proper ones to the native side. Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you Wojtek and I would totally go into using Spec as the type only for codegen components, instead of reusing types from types.tsx, but tbh I'd leave this task for a separate PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

But then you wouldn't have to add this RenamedSearchBarProps type at all yeah? But if you want, you can apply it in the follow-up PR. I'd also change all props to have prefix of onSearch... to keep the convention and not stumble to some similar issue later.

Copy link
Member

Choose a reason for hiding this comment

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

So the conclusion would be to:

  1. leave types in types.tsx intact (so the public API of our lib)
  2. Make NativeSearchBar use prop types that do come from Fabric spec, not from the types.tsx
  3. in SearchBar component hijack these event-related props: onBlur & onFocus and pass them to as values to onSearchBlur & onSearchFocus props on NativeSearchBar respectively

This way we avoid the need for this typescript hacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @WoLewicki @kkafar, I've just added a change that doesn't impact types file and does a little bit tricky stuff inside the SearchBar, but I think this is fine 😄. Could you tell me what do you think about it? 299ce6a

Loading