Fix: make company card CSV import help links reliably tappable on Android - #99727
Fix: make company card CSV import help links reliably tappable on Android#99727MelvinBot wants to merge 9 commits into
Conversation
…roid Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Fixed the failing Oxfmt check. Cause: Fix: Broke that object literal across multiple lines (matching The only other non-passing check, |
|
🤖 Android test result: I ran the PR's test steps on standalone Android but was blocked before reaching the screen under test. Tapping Add cards triggers a mandatory "Validate your account" screen that requires a one-time 6-digit security code emailed to the test account ( I did get the environment ready: font scale was confirmed set to minimum, a workspace was created, Company cards was enabled, and I navigated to Add cards — where the validation wall appeared.
Why: Adding a company card requires account validation via a one-time code that only lands in the test account's mailbox, which isn't accessible to the tester after session bring-up. To verify these help links on Android, the test account's contact method would need to be pre-validated before the session starts, or the tester would need a way to read the validation code for that mailbox. Evidence (2)Workspaces list screen
Validate your account screen blocking the Add cards flow with a 6-digit emailed security code requirement
|
|
The Oxfmt check is now passing — the formatting fix I pushed earlier ( The only check still showing as failing is (ESLint and the perf tests were still running when I checked; my change was a whitespace-only reflow, so it doesn't affect them.) |
|
🤖 Web test result: I ran the PR's test steps on Web but was blocked before reaching the screen under test — the same wall I hit on Android. Clicking Add cards on the Company cards page triggers a mandatory "Validate your account" screen requiring a one-time 6-digit code emailed to the test account ( I did get there cleanly: created a workspace, opened Workspace settings → Company cards → Add cards — where the validation wall appeared.
Why: Adding a company card requires account validation via a one-time code that only lands in the test account's mailbox, which isn't accessible to the tester after session bring-up. To verify these help links (on either platform), the test account's contact method would need to be pre-validated before the session starts, or the tester would need a way to read the validation code for that mailbox. Evidence (2)Workspace settings > Company cards page with Add cards button
Validate your account magic-code screen blocking the Add cards flow
|
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Fixed the failing ESLint check. Cause: The two inline links now render as Fix: Added a The only remaining non-passing check, |
|
No code fix is needed — the real failure (the Oxfmt check) was already resolved by my earlier line-length reflow commit, and it's now passing along with the test suite and Bun tests. The only remaining non-passing check is |
|
The only PR-related check failure was already resolved — the remaining ESLint failure is a pre-existing What happened
Why it's unrelated to this PR Root cause: Bottom line: this PR's own lint issue is fixed. The remaining failure is a separate |
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
@MelvinBot please address the two inline findings. I’ll verify both links on Android at minimum font size afterward.
| > | ||
| <Text style={[styles.textSupporting, styles.link]}>{segment.text}</Text> | ||
| </PressableWithoutFeedback> | ||
| <Text style={styles.textSupporting}> </Text> |
There was a problem hiding this comment.
This inserts a space after every link, which changes Japanese and Chinese copy, for example テンプレートをダウンロード するか and 帮助指南 。. Could spacing remain defined by each translated segment?
There was a problem hiding this comment.
Good catch — fixed in 865a6ba. The hardcoded <Text> </Text> after each link was the culprit: it injected a space regardless of locale, which is wrong for CJK where the translated segments carry no spaces (hence テンプレートをダウンロード するか and 帮助指南 。).
I removed that hardcoded space and now let each translated segment define its own spacing. The plain-copy tokenizer keeps each word's own leading/trailing whitespace (/\s*\S+\s*/g instead of /\S+\s*/g, which dropped leading spaces), so:
- English still wraps/spaces correctly via
instructionStart(trailing space),instructionMiddle(or check out our), andinstructionEnd(leading space). - Japanese/Chinese get no injected spaces, since those segments contain none — e.g.
…アップロードする前に、テンプレートをダウンロードするか、ヘルプガイドをご確認ください。and…帮助指南。.
| role={segment.role} | ||
| accessibilityLabel={segment.text} | ||
| sentryLabel={segment.sentryLabel} | ||
| onPress={segment.onPress} |
There was a problem hiding this comment.
Could this also pass href={CONST.COMPANY_CARDS_CREATE_FILE_FEED_HELP_URL} to the Pressable? Comparable Pressable links preserve both href and onPress, retaining native web link behavior.
There was a problem hiding this comment.
Done in 865a6ba. The help-guide link now passes href={CONST.COMPANY_CARDS_CREATE_FILE_FEED_HELP_URL} to the PressableWithoutFeedback, so on web it renders as a real <a> (native link behavior — hover URL, right-click/open in new tab, middle-click). onPress calls event?.preventDefault() before openLink(...) so the anchor's default navigation doesn't double-fire, and it still routes through openLink on native. This mirrors the existing Socials.tsx pattern.
Only the help-guide link gets href — the "Download our template" link is a client-side CSV download (no URL), so it stays a plain button.
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Addressed both inline findings and pushed 1. Locale spacing (line 132 thread) — Removed the hardcoded 2. Local checks pass: ESLint, TypeScript, React Compiler compliance, and cspell all green. I didn't run the Android device verification since you're planning to verify both links at minimum font size yourself — ping me if you'd like me to run it instead. |
|
The Oxfmt check I fixed earlier is now green. The remaining real failure, ESLint, is not caused by this PR — it's a pre-existing issue in three files this PR doesn't touch, so I did not change anything this time. What's failing: 3 ×
Root cause: All three pass How I confirmed it isn't this PR
Suggested fix (separate from this PR): update the three call sites to not pass the promise into a void slot, e.g. The only other non-passing checks — |
|
The remaining ESLint check failure is unrelated to this PR — I'm not pushing a fix for it. This PR only touches These are pre-existing violations (e.g. The other two red checks are the I already fixed the earlier Oxfmt failure; nothing further to push here. |
|
@MelvinBot I retested both links on Android at minimum font size. The template downloads and the help guide opens reliably, including edge taps. The fix looks good. Please complete the test/QA steps, author checklist, and platform evidence before marking the PR ready. |
| // (e.g. Japanese, Chinese) aren't given extra spaces the translation never intended. | ||
| return (segment.text.match(/\s*\S+\s*/g) ?? []).map((word, wordIndex) => ( | ||
| <Text | ||
| // eslint-disable-next-line react/no-array-index-key |
There was a problem hiding this comment.
NAB: The two new react/no-array-index-key suppressions lack justification. Could we use a stable segment key and explain why the word-level index is safe?
There was a problem hiding this comment.
Done in c2a16e2:
- Stable segment key — the
PressableWithoutFeedbacklinks now key onsegment.textalone. Each link segment's translated text is unique (template vs. help guide), so the array index is no longer needed and that suppression is gone. - Word-level index — this one genuinely needs the index (repeated words within a segment can't be disambiguated by text alone), so I kept it but justified it inline: the word list is derived synchronously from a fixed translation and is never reordered, inserted into, or filtered, so a word's array position is a stable identity. The justification is now on the disable directive itself:
ImportFromFileStep.tsx:153.
ESLint passes clean on the file.
|
🤖 Thanks for confirming the fix works on your device! Two parts to this: Android platform evidence:
Test/QA steps and author checklist: these need the human co-author. The Evidence (1)Validate your account screen blocking the Add cards flow, requesting an emailed 6-digit security code
|
|
@MelvinBot please address this review comment. |
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
@MelvinBot please address these findings.
| // On Android, a link nested inline inside a <Text> becomes a ClickableSpan whose touch area is limited to the glyph bounds, | ||
| // which makes it unreliable to tap (e.g. at the minimum device font size). Rendering each link as its own PressableWithoutFeedback | ||
| // gives it a real native touch target, while splitting the plain copy into words keeps the paragraph flowing/wrapping naturally. | ||
| const createFileFeedHelpTextSegments: Array<{ |
There was a problem hiding this comment.
Could we render these five fixed segments explicitly instead of building a static config array and mapping it? This is fixed UI, so explicit JSX follows the composition-over-configuration standard and removes the generic segment schema.
There was a problem hiding this comment.
Done in 6a6ed3c.
Removed the createFileFeedHelpTextSegments config array (and its generic segment schema) and now render the five fixed segments as explicit JSX: the three plain-copy runs via a small renderPlainCopy helper (kept only to DRY the per-word wrapping + its no-array-index-key justification), and the template/help-guide links as their own explicit PressableWithoutFeedback. Composition over configuration, no runtime behavior change.
| {translate('workspace.companyCards.addNewCard.createFileFeedHelpText.instructionEnd')} | ||
| </Text> | ||
| <View style={[styles.ph5, styles.mv3, styles.flexRow, styles.flexWrap, styles.alignItemsCenter]}> | ||
| {createFileFeedHelpTextSegments.map((segment) => { |
There was a problem hiding this comment.
Could we add focused coverage confirming both inline actions remain Pressable-backed and the help guide retains its href? These new branches are currently uncovered.
There was a problem hiding this comment.
Added in 6a6ed3c — new tests/unit/ImportFromFileStepTest.tsx renders the step and covers both inline actions:
- Template link — asserts it renders as a
PressableWithoutFeedback(role="button") and that pressing it triggers the client-side CSV download (localFileDownload). - Help guide link — asserts it stays a
PressableWithoutFeedback(role="link") that retainshref={CONST.COMPANY_CARDS_CREATE_FILE_FEED_HELP_URL}, and that pressing it routes throughopenLink.
Both tests pass locally.
…ink coverage Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Addressed both review findings and pushed 1. Render the five fixed segments explicitly (line 73 thread) — Removed the 2. Focused coverage for the inline actions (line 130 thread) — Added
Local checks all pass: the new Jest suite (2/2), ESLint, TypeScript, React Compiler compliance, and cspell. |
| // On Android, a link nested inline inside a <Text> becomes a ClickableSpan whose touch area is limited to the glyph bounds, | ||
| // which makes it unreliable to tap (e.g. at the minimum device font size). Rendering each link as its own PressableWithoutFeedback | ||
| // gives it a real native touch target, while splitting the plain copy into words keeps the paragraph flowing/wrapping naturally. | ||
| const renderPlainCopy = (text: string) => |
There was a problem hiding this comment.
@MelvinBot could we extract renderPlainCopy into a small WrappingText component? It reshapes translated text and returns JSX while closing over the page styles, so the helper still hides part of the render tree.
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Done — extracted
Verified locally: ESLint, |
|
@MelvinBot please fix the PR-caused Oxfmt and knip failures in |
…oc header Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Fixed the two PR-caused failures in
The ESLint failure is unrelated, as you noted — its 3 errors are all Verification noteI couldn't run If "the required file header" meant something other than a JSDoc doc header (e.g. a specific license/copyright banner), let me know — I couldn't find any such convention in the repo (no source file carries one), so I went with the JSDoc header. |
|
|
||
| import React from 'react'; | ||
|
|
||
| type WrappingTextProps = { |
There was a problem hiding this comment.
NAB: @MelvinBot the JSDoc documents the component, but the new file still has no header before WrappingTextProps. Please add a short file description above the type.
Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Done — added a short file description above The existing JSDoc still documents the component; the new header gives the file a quick top-of-file summary before the type. ESLint passed on the file (the repo's prettier sort-imports plugin isn't installed in this environment, but the change is a comment-only insertion that doesn't touch imports or formatting). |
|
@nyomanjyotisa are you around to get this ready for review now? |
|
Closing in favor of #99783 |
3 participants
Explanation of Change
Fixes the deploy blocker where the inline "help guide" (and "Download our template") links on the company-card CSV import step were not tappable on Android at the minimum device font size.
The help text was rendered as a single
<Text>with two bare<TextLink>children nested inline. On Android, a link nested inside a<Text>becomes a nativeClickableSpanwhose touch area is limited to the glyph bounds, so it can become unreliable to tap (reproduced at the minimum device font size). This is the same rendering an<a>gets throughRenderHTMLfor non-comment links, so switching back toRenderHTMLwould not have fixed it — andRenderHTMLcan't invoke the client-side template download that #98058 requires.This change keeps both links inline (preserving the inline template link required by #98058) but renders each link as its own
PressableWithoutFeedback, which gives it a real native touch target. The surrounding copy is split into words inside aflexWraprow so the paragraph still flows and wraps naturally. This mirrors the existingTextLinkBlockapproach already used in the app for reliably tappable link text.Fixed Issues
$ #99719
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Suggested (needs a validated account to reach the screen): set the Android device font size to minimum, then go to Workspace settings > Company cards > Add cards > United States > Next > Import transactions from files > Next. Verify the inline "Download our template" link downloads the CSV template and the inline "help guide" link opens the help guide, and that the paragraph still reads/wraps normally.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari