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

[RNMobile] Fix BottomSheet.SubSheet/TextInput Conflict #33845

Merged
merged 4 commits into from
Aug 3, 2021
Merged

[RNMobile] Fix BottomSheet.SubSheet/TextInput Conflict #33845

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2bc7ec
Remove 'BottomSheetScreen' component
Aug 3, 2021
e53d768
Refactor 'setIsFullScreen' method
Aug 3, 2021
5232398
Remove 'BottomSheetConsumer'
Aug 3, 2021
b75e766
Refactor reference to 'children'
Aug 3, 2021
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
27 changes: 10 additions & 17 deletions packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { SafeAreaView } from 'react-native';
/**
* WordPress dependencies
*/
import { Children, useEffect } from '@wordpress/element';
import { createSlotFill, BottomSheetConsumer } from '@wordpress/components';
import { Children, useEffect, useContext } from '@wordpress/element';
import { createSlotFill, BottomSheetContext } from '@wordpress/components';

const { Fill, Slot } = createSlotFill( 'BottomSheetSubSheet' );

Expand All @@ -17,26 +17,19 @@ const BottomSheetSubSheet = ( {
showSheet,
isFullScreen,
} ) => {
const BottomSheetScreen = ( { onMount } ) => {
useEffect( onMount, [] );
return children ?? null;
};
Comment on lines -20 to -23
Copy link
Member

@dcalhoun dcalhoun Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding from the PR description and testing is that the creation of this component within the render of its parent BottomSheetSubSheet is the root cause of the issue, correct?

I am not suggesting we change this PR, but I wanted to share the below alternative that is closer to the original implementation, purely for discussion's sake. It would appear merely hoisting BottomSheetScreen out of BottomSheetSubSheet does fix the original keyboard/focus issue.

Alternative Approach 
diff --git a/packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js b/packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js
index 8654dc918e..2af2c0f5da 100644
--- a/packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js
+++ b/packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js
@@ -11,17 +11,17 @@ import { createSlotFill, BottomSheetConsumer } from '@wordpress/components';
 
 const { Fill, Slot } = createSlotFill( 'BottomSheetSubSheet' );
 
+const BottomSheetScreen = ( { children, onMount } ) => {
+	useEffect( onMount, [] );
+	return children;
+};
+
 const BottomSheetSubSheet = ( {
 	children,
 	navigationButton,
 	showSheet,
 	isFullScreen,
 } ) => {
-	const BottomSheetScreen = ( { onMount } ) => {
-		useEffect( onMount, [] );
-		return children ?? null;
-	};
-
 	return (
 		<>
 			{ showSheet && (
@@ -33,7 +33,9 @@ const BottomSheetSubSheet = ( {
 									onMount={ () =>
 										setIsFullScreen( isFullScreen )
 									}
-								/>
+								>
+									{ children }
+								</BottomSheetScreen>
 							) }
 						</BottomSheetConsumer>
 					</SafeAreaView>

Edit: Updated the diff to showcase the difference between the original implementation, instead of the difference between this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this, David! 🙇‍♀️ You're right on what I believe to be the root cause of this issue (i.e. the creation of the child component within the parent). I appreciate you sharing that alternative approach, as hoisting the component isn't something I'd have come up with by myself, so will add it to my knowledge bank going forward.

Are there any positives or negatives to either approach? I'm definitely open to learning and updating the PR if there is anything steering you towards or away from either approach. If it's the case that they're fairly similar, with no major advantages/disadvantages to one another, I'll go ahead to merge the current PR.

Copy link
Member

Choose a reason for hiding this comment

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

Are there any positives or negatives to either approach? I'm definitely open to learning and updating the PR if there is anything steering you towards or away from either approach. If it's the case that they're fairly similar, with no major advantages/disadvantages to one another, I'll go ahead to merge the current PR.

No, I wouldn't say there is a distinct positive or negative with either approach. It is merely differently structured code from my perspective.

That said, after reviewing this again, I believe this wrapping BottomSheetConsumer may be duplicative of the new useContext usage and unnecessary. I.e. we could likely remove BottomSheetConsumer altogether.

// React Context accessed via Consumer with function as a child 
<BottomSheetConsumer>
	{({ setIsFullScreen }) => {
		// ...
	}}
</BottomSheetConsumer>

// React Context accessed via useContext Hook
const { setIsFullScreen } = useContext(BottomSheetContext);

For an explanation as to why, both of the above are accessing the React Context object. The former is using the Context.Consumer approach. The latter is using the React Hook approach. Either is fine. I believe this project generally attempts to rely upon React Hooks now-a-days.

Currently, this PR has a lingering BottomSheetConsumer that is largely going unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, excellent, that's clear, thanks for the explanation! I've gone ahead to remove code surrounding that unnecessary BottomSheetConsumer in 5232398 and b75e766. I also tested and can confirm the alt text settings still work with the update.

const { setIsFullScreen } = useContext( BottomSheetContext );

useEffect( () => {
if ( showSheet ) {
setIsFullScreen( isFullScreen );
}
}, [ showSheet, isFullScreen ] );

return (
<>
{ showSheet && (
<Fill>
<SafeAreaView>
<BottomSheetConsumer>
{ ( { setIsFullScreen } ) => (
<BottomSheetScreen
onMount={ () =>
setIsFullScreen( isFullScreen )
}
/>
) }
</BottomSheetConsumer>
</SafeAreaView>
<SafeAreaView>{ children }</SafeAreaView>
</Fill>
) }
{ Children.count( children ) > 0 && navigationButton }
Expand Down