feat(components-native): Add underline support to Text/Typography components for mobile (JOB-104505)

[sabidhasan]

Motivations

This PR adds the ability to add underlining to the Text component for the native-components.

Visual Changes

Android

iOS

Discussion

The underline styles here (dashed, etc) are not implemented yet on the Android side. For this reason Android defaults to the basic underline style regardless of what value is passed in. For iOS, the proper dashed style should be displayed.

Added

• Added underline prop to TextProps with options "solid" or "dashed"
• Added underline prop to TypographyProps with options "solid", "double", "dotted", or "dashed" (I covered more than what is exposed from Text, because this is an internal component; if the reviewer thinks this is unnecessary, I can change it to support only the solid/dashed cases that we need)
• Added underline styles to typographyTokens

Testing

To test locally, run npm run pack @jobber/components-native and then install into the mobile repo via npm i /path/to/atlantis/jobber-components-native-x.x.x.tgz. Then change a Text component to pass an underline prop value.

For the screenshots above, I used the branch JOB-103929-jobber-mobile-implement-explainability-ui at commit 2c1c74b7c4c9f7b5ae569f4121c3505968e9bed3 and just locally installed the version of Atlantis that was packed from the command above.

Changes can be tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[sabidhasan]

Adding the underline to the text looks good, but it appears that the we've changed the spacing between the top of the message bubble and the first line of text? Is it because the react native text is missing padding that the message previously had?

Minor nit, the "Copilot may rephrase..." text doesn't need a period.

I wouldn't worry about that - that is coming from the container styles, so shouldn't be related to the text itself I don't think.

[jlelford]

I wouldn't worry about that - that is coming from the container styles, so shouldn't be related to the text itself I don't think.

So it shouldn't be concerned that the spacing appears to be inconsistent in the Copilot response? (Pixel measurements are just to indicate they're the same size squares in each message)

[sabidhasan]

I wouldn't worry about that - that is coming from the container styles, so shouldn't be related to the text itself I don't think.

So it shouldn't be concerned that the spacing appears to be inconsistent in the Copilot response? (Pixel measurements are just to indicate they're the same size squares in each message)

Correct, the spacing there is set on the Markdown component's container, and for this screenshot I didn't do that; here it is with the margin set. Actually in looking at this a bit more, I discovered we are probably setting the wrong margin on the container as-is: the library sets the paragraph margins to 10px and I assume we want 8 or 12?

Regardless, if we set them to 10 here, we get the same spacing:

[jlelford]

Correct, the spacing there is set on the Markdown component's container, and for this screenshot I didn't do that; here it is with the margin set. Actually in looking at this a bit more, I discovered we are probably setting the wrong margin on the container as-is: the library sets the paragraph margins to 10px and I assume we want 8 or 12?

Ideally yes, would that then impact 2way sms? I'm a bit hesitant to add more scope here changing and reviewing this though given our timelines.

[sabidhasan]

Correct, the spacing there is set on the Markdown component's container, and for this screenshot I didn't do that; here it is with the margin set. Actually in looking at this a bit more, I discovered we are probably setting the wrong margin on the container as-is: the library sets the paragraph margins to 10px and I assume we want 8 or 12?

Ideally yes, would that then impact 2way sms? I'm a bit hesitant to add more scope here changing and reviewing this though given our timelines.

No nothing we do would impact 2WSMS - we are configuring gifted chat separately from them.

[cloudflare-workers-and-pages[bot]]

Deploying atlantis with    Cloudflare Pages

Latest commit:	e9900e0
Status:	✅  Deploy successful!
Preview URL:	https://f893d72e.atlantis.pages.dev
Branch Preview URL:	https://job-104505-add-underlining-t.atlantis.pages.dev

View logs

[github-actions[bot]]

Published Pre-release for f142a3c38918be56d7fd5af3d679c0940f89c51c with versions:

  - @jobber/components-native@0.71.3-JOB-104505-f142a3c.4+f142a3c3

To install the new version(s) for Mobile run:

npm install @jobber/components-native@0.71.3-JOB-104505-f142a3c.4+f142a3c3

[chris-at-jobber]

@jlelford and I discussed "offline" regarding the desired experience for consumers regarding setting underline color

Because there are presently no usages of the underline property as it's a net-new, I'm OK with the hard-coded color approach we're using here as a means to unblock, but I believe the use case in this PR (underline color ≠ text color) is likely the minority use case, and this PR should be followed with

• a new default that changes the underline color to match the specified Typography color
• adding the ability for Typography consumers to specify the underline color
  • (ideally/potentially as an addition to the underline property, like underline: dashed interactive--subtle so that we're not adding another prop that Android is not even aware of)
• then updating the Copilot usages to leverage Typography with custom underline color

[sabidhasan]

@jlelford and I discussed "offline" regarding the desired experience for consumers regarding setting underline color

Because there are presently no usages of the underline property as it's a net-new, I'm OK with the hard-coded color approach we're using here as a means to unblock, but I believe the use case in this PR (underline color ≠ text color) is likely the minority use case, and this PR should be followed with

• a new default that changes the underline color to match the specified Typography color

• adding the ability for Typography consumers to specify the underline color

  • (ideally/potentially as an addition to the underline property, like underline: dashed interactive--subtle so that we're not adding another prop that Android is not even aware of)
• then updating the Copilot usages to leverage Typography with custom underline color

@chris-at-jobber a ticket for this work has been made here: https://jobber.atlassian.net/browse/JOB-104571

[sabidhasan]

LGTM, but I do notice that the Text component accepts the same values for underline as the Typography component, even though they don't appear on the type. I dunno how big of a deal that is.

Might an IDE or language server problem, but the type specified for text is not the same as Typography, and at least for me I only see the types I should: