[Mobile] Add external link inline component #33210
Conversation
enejb
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
enejb
added
the
Mobile App - i.e. Android or iOS
enejb
added
[Block] Heading
|
Size Change: +24.7 kB (+2%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for putting this together! From a visual standpoint, this looks good to me. I tested on an iPhone 12 mini Simulator and Samsung Galaxy S20. Removing FooterMessageLink is also a really good idea IMO.
I left several comments regarding accessibility concerns. I do understand some of them preexisted, so if you would like to ship this visual fix as-is with the accessibility issues, I understand and support that. However, we should at least create a new issue for the accessibility issues if they do not already exist. WDYT?
If you would prefer me to create the new issues, just let me know and I can. Full transparency — it'll probably be a few days until I find the time to create them.
There was a problem hiding this comment.
Thank you for looping me in here @enejb! I hadn't spotted the <ExternalLink> component and agree it makes a lot more sense to re-use it vs. the <FooterMessageLink> component.
I tested on a Pixel 3 emulator and confirm that everything looks good visually to me. I was able to replicate the Object Object screenreader problem that @dcalhoun reported, however. Let me know if you'd like me to create the GitHub issue for that, as I appreciate it was ultimately me that introduced that bug with the way I set up the <FooterMessageLink> component.
The only additional suggestion I had was around expanding on the current README for ExternalLink. That's a very small suggestion, but may be useful for clarity and might help to make the component more discoverable for folks quickly browsing through.
packages/components/src/mobile/bottom-sheet/footer-message-link/README.md
Show resolved
Hide resolved
enejb
force-pushed
the
rn/add/external-link-inline
branch
from
d9a4fb4
to
0ad0f01
Compare
enejb
changed the title
Add a space character to give the link some space.
enejb
force-pushed
the
rn/add/external-link-inline
branch
from
0ad0f01
to
b9c0c64
Compare
As with the previous commit, the necessary space between text and link in the HelpText component is added here.
This README has been updated since this PR was first worked on (in #41681). As such, this commit brings in the latest updates for accuracy.
|
I've gone ahead to update this branch and made some tweaks to resolve errors against A noteworthy change is that I updated the Social Icons Link Setting ⤵
Empty Embed Block's Link Setting ⤵
@dcalhoun, I've gone ahead to request your reviews due to your previous involvement on this PR, but let me know if you're not available or if you'd rather someone else take the review. Thanks! 🙇♀️ |
SiobhyB
requested review from
dcalhoun
and removed request for
juanmaguitar,
ajitbohra,
chrisvanpatten,
ryanwelcher,
ellatrix and
fabiankaegy
Hey @SiobhyB. 👋🏻 I have begun reviewing this again, but I will likely need a week or more to complete my review as I will be AFK for a bit. I plan to follow up middle of next week, but feel free to seek other help if you'd prefer. |
Thank you, David! That sounds good, I don't think there's an urgency to this PR. 🙇♀️ |
There was a problem hiding this comment.
Hey @SiobhyB. 👋🏻 I appreciate your patience. Returning to this PR was challenging to regain the context required to provide (hopefully) valuable feedback.
Add inline link to an external resource in the mobile app. It replaces the current
FooterMessageLinkComponent since it does the same thing as the external link to create consistency with the Gutenberg components code base.
My review is quite long. I hope it is not overwhelming. I felt this type of refactor introducing new paradigms to the code base was worthy of a long look. The scope of this PR seems to have grown past the original goal of replacing FooterMessageLink with an external link component, presumably to address the accessibility label issues.
Some comments call out specific issues we should address before merging. However, other of the comments are pretty open-ended. As mentioned in those opened-ended comments, it is fine if we'd prefer to move forward with the current approach instead.
Thanks!
| onPress={ () => Linking.openURL( href ) } | ||
| accessibilityLabel={ __( 'Open link in a browser' ) } | ||
| 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 typeof children !== 'string'. WDYT?
There was a problem hiding this comment.
Agreed that's a good idea, I've added an error message in 8d4d17e.
| isWeb && ( | ||
| <> | ||
| { __( | ||
| 'Enter a word or two — without spaces — to make a unique web address just for this block, called an “anchor.” Then, you’ll be able to link directly to this section of your page.' | ||
| ) } | ||
| <ExternalLink | ||
| href={ | ||
| 'https://wordpress.org/support/article/page-jumps/' | ||
| } | ||
| > | ||
| { __( 'Learn more about anchors' ) } | ||
| </ExternalLink> | ||
| </> | ||
| ) |
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.
Here the web now relies upon passing help to a robust, composable TextControl → BaseControl component that manages help text and links; native now relies upon HelpText composed further along in this hook file rather than the previous approach of passing help down to TextControl → Cell.
While native's Cell is quite complex and overly far-reaching — it is quite different from the web's composable approach in BaseControl and likely not the best abstraction long term — I am hesitant move the help text from TextControl → Cell to inline HelpText in 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 Object accessibility (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's help property here.
An example of a possible approach might involve flattening help props rather than introducing the HelpText component. The flattened help prop could be used for a11y labels, the original help prop could be rendered as normal for visible UI. Note: I didn't test any performance of such code.
const helpPropExample = (
<>
A top-level sentence.{" "}
<ExternalLink href={"https://wordpress.org/support/article/page-jumps/"}>
A nested link
</ExternalLink>
</>
);
function flattenElementToString(reactElement = "") {
if (!reactElement) {
return;
}
const children = reactElement.props?.children || reactElement;
if (Array.isArray(children)) {
return children.reduce(
(acc, child) => acc + flattenElementsToString(child),
""
);
} else if (React.isValidElement(children)) {
return flattenElementsToString(children);
}
return children;
}
const flatHelpPropExample = flattenElementToString(helpPropExample);
console.log(">>>", flatHelpPropExample); // >>> A top-level sentence. A nested link
<Text accessibilityLabel={flatHelpPropExample}>{helpPropExample}</Text>;| footer: { | ||
| label: ( | ||
| <FooterMessageLink | ||
| href={ __( | ||
| 'https://wordpress.org/support/article/embeds/' | ||
| ) } | ||
| value={ __( 'Learn more about embeds' ) } | ||
| /> | ||
| ), | ||
| help: { | ||
| url: __( 'https://wordpress.org/support/article/embeds/' ), | ||
| moreLinkText: __( 'Learn more about embeds' ), |
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.
The replacement of the composable approach with a prop-driven approach is less than ideal IMO. I.e. the ability to pass React elements means we have more optionality in regards to what we pass without defining additional props. The prop-driven approach limits us to a very specific composition defined within HelpText.
For this specific example of help, this ultimate uses HelpText, but only passes props for a "more link." So, in reality, this only needs to render an ExternalLink, not an entire HelpText. This unnecessary complexity is less than ideal IMO.
My understanding for the rationale for this change is similar to my previous comment, it is difficult to pass both a string and a interactive link and not run into accessibility label issues.
|
|
||
| ## 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 children prop?
Also, I believe this only to be true on native, as it currently applies an onPress to a wrapping Text element. The web version merely renders the child text. I wonder if we can better describe this.
| onPress={ () => { | ||
| AccessibilityInfo.isScreenReaderEnabled().then( | ||
| ( enabled ) => enabled && Linking.openURL( url ) | ||
| ); | ||
| } } |
There was a problem hiding this comment.
Similar to accessibilityRole and accessibilityHint, I wonder if this should be conditionally set if there is a url defined.
onPress={ url ? () => { /* [...] */ } : undefined }| { 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 url in this file. E.g.:
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.
| accessibilityRole={ url ? 'link' : 'text' } | ||
| accessibilityHint={ | ||
| url ? __( 'Opens link in a browser' ) : null | ||
| } |
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 accessibilityLabel={ moreLinkText }. It causes the moreLinkText to be read for both platforms. Unfortunately, it no longer reads the entire help text aloud on iOS.
Using something like accessibilityLabel={ children + moreLinkText } is not possible as it reads aloud "Object Object," presumably because of translation string methods used: __().
| help={ | ||
| <> | ||
| { __( 'A footer note to add to the component! ' ) } | ||
| <FooterMessageLink | ||
| href={ 'https://wordpress.org/' } | ||
| value={ __( 'Visit WordPress.org' ) } | ||
| /> | ||
| <ExternalLink href={ 'https://wordpress.org/' } > | ||
| { __( 'Visit WordPress.org' ) } | ||
| </ExternalLink> | ||
| </> | ||
| } |
There was a problem hiding this comment.
While the original "Object Object" a11y label issue appears to be resolved in the app, I believe example offered here would still result in a problematic a11y label. We likely need to replace the example showcasing an inline link with the usage of HelpText showcased in the changes this PR makes to the Image edit component or change the entire architecture of this PRs approach to composing help texts with links.
Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
A conditional has been created to check that s link has text associated with it before rendering.
|
@dcalhoun, thank you, I really appreciate the thorough review and thoughtfulness you put into ensuring quality. 🙇♀️ I plan to address all of your comments, but likely won't be able to get to them all until after our upcoming meetup. I know you're going to be AFK at that point, so wanted to note here that I'll aim to find someone else to do another review after working through all the points. Thanks again for your help so far! |
|
@dcalhoun, @enejb, this is a backlog item that I never managed to prioritise again, since my last activity a year ago. Weighing up some of the hesitation around the changes and the fact that we haven't actively sought to prioritise this since June, 2021, I'm thinking of closing this PR. What are your thoughts? If we agree, I'd plan to review the accessibility improvements and spin them out into a separate PR. |
My concern regarding composition and alignment with the web, unused code, and lingering accessibility issues remain for me. So, closing the PR and focusing on a subset of the changes is sensible from my perspective. If needed in the future, we could always reopen the PR. |
|
Thank you, @dcalhoun! I'll go ahead to close this and review the subset of changes we can keep as part of my planned maintenance week next week. |
Description
Add inline link to an external resource in the mobile app. It replaces the current
FooterMessageLinkComponent since it does the same thing as the external link to create consistency with the Gutenberg components code base.How has this been tested?
Screenshots
See Header Settings
Image block Alt Text settings.
Types of changes
Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).