Replace react-native-hr library #51041
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derekblank
added
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
dcalhoun
approved these changes
May 30, 2023
There was a problem hiding this comment.
Nice! Thank you for working to remove this dependency. The test cases succeeded for me on an iPhone 14 Pro simulator and Pixel 3 XL emulator.
I left one suggestion for your consideration, but it is by no means a blocker.
NOTE: I started this PR without realizing that a similar effort to remove react-native-hr had already been undertaken in #48986, which remains open. The approach in this PR is essentially a direct reimplementation of the original library code, which may address some of the feedback concerns from #48986 regarding matching web more closely.
@dcalhoun and @geriux -- let's discuss and get one or the other merged either way, as we've now identified twice that this library is completely unnecessary as a third-party fork and can be removed. 😅
Some of my feedback in #48986 (comment) is still relevant in regards to reducing the number of props and component API surface area. However, I completely support postponing that larger refactor to a different time. It was not my intention to delay removing this dependency, but to start a conversation about improvement opportunities. We can postpone those to another time.
One note: #48986 does add support for background color. So, that is one additional user-facing feature we are postponing as well. Not an issue from my perspective, but wanted to make a note of it.
@geriux what do you view as the best path forward? Please do not take my approval of this PR as a final say or that we should discard #48986, feel free to share your thoughts or preference. 🙇🏻
| marginRight={ 0 } | ||
| /> | ||
| > | ||
| { renderInner() } |
There was a problem hiding this comment.
If we desired to use JSX rather than function invocations, we could leverage Hooks and composition to structure the children a bit differently and still avoid prop drilling. Feel free to ignore the suggestion, though.
diff --git a/packages/primitives/src/horizontal-rule/index.native.js b/packages/primitives/src/horizontal-rule/index.native.js
index 8841c8e694..3d3fef91a1 100644
--- a/packages/primitives/src/horizontal-rule/index.native.js
+++ b/packages/primitives/src/horizontal-rule/index.native.js
@@ -6,14 +6,33 @@ import { Text, View } from 'react-native';
/**
* WordPress dependencies
*/
-import { withPreferredColorScheme } from '@wordpress/compose';
+import { usePreferredColorSchemeStyle } from '@wordpress/compose';
/**
* Internal dependencies
*/
import styles from './styles.scss';
-const HR = ( {
+const Line = ( { children } ) => {
+ const lineStyle = usePreferredColorSchemeStyle(
+ styles.line,
+ styles.lineDark
+ );
+
+ if ( children ) {
+ return (
+ <>
+ <View style={ lineStyle } />
+ { children }
+ <View style={ lineStyle } />
+ </>
+ );
+ }
+
+ return <View style={ lineStyle } />;
+};
+
+export const HorizontalRule = ( {
getStylesFromColorScheme,
lineStyle,
marginLeft,
@@ -22,26 +41,6 @@ const HR = ( {
text,
...props
} ) => {
- const renderLine = ( key ) => (
- <View
- key={ key }
- style={ getStylesFromColorScheme( styles.line, styles.lineDark ) }
- />
- );
-
- const renderText = ( key ) => (
- <View key={ key }>
- <Text style={ [ styles.text, textStyle ] }>{ text }</Text>
- </View>
- );
-
- const renderInner = () => {
- if ( ! text ) {
- return renderLine();
- }
- return [ renderLine( 1 ), renderText( 2 ), renderLine( 3 ) ];
- };
-
return (
<View
style={ [
@@ -51,9 +50,11 @@ const HR = ( {
] }
{ ...props }
>
- { renderInner() }
+ <Line>
+ { text ? (
+ <Text style={ [ styles.text, textStyle ] }>{ text }</Text>
+ ) : null }
+ </Line>
</View>
);
};
-
-export const HorizontalRule = withPreferredColorScheme( HR );
|
@dcalhoun Thanks for the review! As this PR is essentially identical to the source of react-native-hr (with a few code style changes), perhaps we could merge this one to remove the dependency immediately for the RN upgrade project, and then address the JSX change suggestion and your other feedback from #48986 (including the background color change) in a follow-up PR? @geriux WDYT? |
|
Flaky tests detected in e912f05. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5140748129
|
geriux
approved these changes
Jun 1, 2023
|
Now that we no longer use the forked repository of NOTE: I tried to archive it but looks like I don't have permissions 🤔. Could someone try to archive it? Thanks! |
Done! ✅ |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Update native HorizontalRule primitive component * Remove react-native-hr package references
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Replaces the forked version of react-native-hr in favor of implementing the library within Gutenberg instead.
NOTE: I started this PR without realizing that a similar effort to remove react-native-hr had already been undertaken in #48986, which remains open. The approach in this PR is essentially a direct reimplementation of the original library code, which may address some of the feedback concerns from #48986 regarding matching web more closely.
@dcalhoun and @geriux -- let's discuss and get one or the other merged either way, as we've now identified twice that this library is completely unnecessary as a third-party fork and can be removed. 😅
Why?
To reduce the number of third-party libraries and dependencies in order to support React Native's New Architecture.
How?
The PR takes the source of our forked react-native-hr code, which is just one single JS file, and reimplements the source within the HorizontalRule primitive (while converting the code from a class component to a function component in the process). Then, the files that reference the primitive are updated with the new internal dependency.
Testing Instructions
trunk(or production) is identicalScreenshots or screencast