[Mobile] Add external link inline component #33210
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,23 +2,42 @@ | |
| * External dependencies | ||
| */ | ||
|
|
||
| import { Text, Linking } from 'react-native'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { usePreferredColorSchemeStyle } from '@wordpress/compose'; | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import style from './style.native.scss'; | ||
|
|
||
| export function ExternalLink( { href, children } ) { | ||
| const externalLink = usePreferredColorSchemeStyle( | ||
| style[ 'external-link' ], | ||
| style[ 'external-link__dark' ] | ||
| ); | ||
|
|
||
| if ( typeof children !== 'string' ) { | ||
| throw new Error( | ||
| __( | ||
| 'The ExternalLink component only accepts a string as a child element. This is to ensure the element can be read by screen readers.' | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Text | ||
| style={ externalLink } | ||
| onPress={ () => Linking.openURL( href ) } | ||
| accessibilityLabel={ children } | ||
|
There was a problem hiding this comment. To avoid future a11y label issues, it may be worth throwing an error in this component if There was a problem hiding this comment. Agreed that's a good idea, I've added an error message in 8d4d17e. |
||
| accessibilityHint={ __( 'Opens link in a browser' ) } | ||
| accessibilityRole={ 'link' } | ||
| > | ||
| { children } | ||
| </Text> | ||
| ); | ||
| } | ||
|
|
||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .external-link { | ||
| color: $blue-50; | ||
| } | ||
|
|
||
| .external-link__dark { | ||
| color: $blue-30; | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # HelpText | ||
|
|
||
| Renders help text with a `ExternalLink` appended at the end of the text. | ||
|
|
||
| ## Usage | ||
|
|
||
| ```jsx | ||
| import { HelpText } from '@wordpress/components'; | ||
|
|
||
| const HelpfulText = () => ( | ||
| <HelpText url="https://wordpress.org" moreLinkText="Learn More">WordPress is an easy to use publishing tool</HelpText> | ||
| ); | ||
| ``` | ||
|
|
||
| ## Props | ||
|
|
||
| The text that's displayed within the component's tags will be "clickable" and extenal link will be added at the end. | ||
|
There was a problem hiding this comment. This statement feels out of place underneath the heading of "Props." Was this intended to be a description of the Also, I believe this only to be true on native, as it currently applies an |
||
|
|
||
| ### `url` | ||
|
|
||
| An optional URL to link, opens in a new window when tapped or clicked. | ||
|
|
||
| - Type: `String` | ||
| - Required: No | ||
|
|
||
| ### `moreLinkText` | ||
|
|
||
| An optional text string to display as the link label for the optional `url` prop. | ||
|
|
||
| - Type: `String` | ||
| - Required: No | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { ExternalLink } from '../external-link'; | ||
|
|
||
| /** | ||
| * @param {Object} props | ||
| * @param {string} props.moreLinkText | ||
| * @param {import('react').ReactNode} props.children | ||
| * @param {string} props.url | ||
| */ | ||
| const HelpText = ( { moreLinkText, children, url } ) => { | ||
|
There was a problem hiding this comment. 🤔 This comment is quite broad and provides more questions than solutions. I understand if we'd prefer to move forward without addressing this. This is the first time I have encountered us adding a web component. That may be OK, but it is a bit odd to add it when there is no usage of it anywhere. For the most part, I've seen the web project lead, and the native project sometimes splits existing web components. Additionally, there is a I raise this primarily as it is largely uncharted territory for me. WDYT? |
||
| return ( | ||
| <div> | ||
| { children } | ||
| { children && url && ' ' } | ||
| { url && ( | ||
| <ExternalLink href={ url }>{ moreLinkText }</ExternalLink> | ||
| ) } | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default HelpText; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { View, Text, AccessibilityInfo, Linking } from 'react-native'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { ExternalLink } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { usePreferredColorSchemeStyle } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import styles from './styles.scss'; | ||
|
|
||
| const HelpText = ( { moreLinkText, children, url, separatorType } ) => { | ||
| if ( typeof children === 'string' ) { | ||
| children = ( | ||
| <Text style={ styles[ 'help-text__text' ] }>{ children }</Text> | ||
| ); | ||
| } | ||
|
|
||
| const separatorStyle = usePreferredColorSchemeStyle( | ||
| styles[ 'help-text__separator' ], | ||
| styles[ 'help-text__separator--dark' ] | ||
| ); | ||
|
|
||
| const hasMoreLink = url && moreLinkText; | ||
|
|
||
| return ( | ||
| <> | ||
| { separatorType === 'topFullWidth' && ( | ||
| <View style={ separatorStyle } /> | ||
| ) } | ||
| <View style={ styles[ 'help-text' ] }> | ||
| <Text | ||
| accessibilityRole={ url ? 'link' : 'text' } | ||
| accessibilityHint={ | ||
| url ? __( 'Opens link in a browser' ) : null | ||
| } | ||
|
Comment on lines
+39
to
+42
There was a problem hiding this comment. This combination of props along with the children text appear to work for iOS. However, it does not appear to read any label loud for Android. From testing briefly, a quick fix is adding Using something like |
||
| onPress={ () => { | ||
| AccessibilityInfo.isScreenReaderEnabled().then( | ||
| ( enabled ) => enabled && Linking.openURL( url ) | ||
| ); | ||
| } } | ||
|
Comment on lines
+43
to
+47
There was a problem hiding this comment. Similar to onPress={ url ? () => { /* [...] */ } : undefined } |
||
| style={ styles[ 'help-text__text' ] } | ||
| > | ||
| { children } | ||
| { children && hasMoreLink && ' ' } | ||
| { hasMoreLink && ( | ||
| <ExternalLink href={ url }> | ||
| { moreLinkText } | ||
| </ExternalLink> | ||
| ) } | ||
| </Text> | ||
| </View> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default HelpText; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| .help-text { | ||
| flex-direction: row; | ||
| min-height: 48; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
| .help-text__text { | ||
| font-size: 12px; | ||
| color: $gray; | ||
| } | ||
|
|
||
| .help-text__separator { | ||
| background-color: $light-gray-400; | ||
| height: 1px; | ||
| } | ||
|
|
||
| .help-text__separator--dark { | ||
| background-color: $gray-70; | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .help-text { | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| .help-text__text { | ||
| color: $gray; | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 This comment is quite broad and provides more questions than solutions. I understand if we'd prefer to move forward without addressing this.
Here the web now relies upon passing
helpto a robust, composableTextControl→BaseControlcomponent that manages help text and links; native now relies uponHelpTextcomposed further along in this hook file rather than the previous approach of passinghelpdown toTextControl→Cell.While native's
Cellis quite complex and overly far-reaching — it is quite different from the web's composable approach inBaseControland likely not the best abstraction long term — I am hesitant move the help text fromTextControl→Cellto inlineHelpTextin this hook as it further diverges web and native component design, which is unfortunate.My interpretation for the rationale of this change is that we cannot pass a multiple children types — string and interactive link — on native. Doing so leads to the
Object Objectaccessibility (a11y) label issue. My hope is that we could identify a way to address the a11y label issue while also avoiding the divergence from web'shelpproperty here.An example of a possible approach might involve flattening
helpprops rather than introducing theHelpTextcomponent. The flattenedhelpprop could be used for a11y labels, the originalhelpprop could be rendered as normal for visible UI. Note: I didn't test any performance of such code.