[Mobile] Add external link inline component #33210
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,23 +2,34 @@ | |
| * 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' ] | ||
| ); | ||
|
|
||
| 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> | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| 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; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # HelpText | ||
|
|
||
| Renders a help text shell with a ExternalLink appended at the end. | ||
SiobhyB marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## 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 that is being linked to and will open in a new window when tapped or clicked. | ||
SiobhyB marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - Type: `String` | ||
| - Required: No | ||
|
|
||
| ### `moreLinkText` | ||
|
|
||
| An optional Text as the click abel text at the end. | ||
SiobhyB marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - Type: `String` | ||
| - Required: No | ||
| 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; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /** | ||
| * 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' ] | ||
| ); | ||
|
|
||
| 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 && url && ' ' } | ||
| { url && ( | ||
|
There was a problem hiding this comment. To avoid the possibility of a link without text, I wonder if we should create a compound conditional for all checks of const hasMoreLink = url && moreLinkText
// [...]
{ children && hasMoreLink && ' ' }
{ hasMoreLink && ( /* [...] */ ) }There was a problem hiding this comment. I appreciate that thought, gone ahead to implement your suggestion in 8ee995c. |
||
| <ExternalLink href={ url }> | ||
| { moreLinkText } | ||
| </ExternalLink> | ||
| ) } | ||
| </Text> | ||
| </View> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default HelpText; | ||
| 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; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .help-text { | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| .help-text__text { | ||
| color: $gray; | ||
| } |
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.