feat(components): Add underline support to Typography component for web (JOB-104497)

[sabidhasan]

Motivations

This PR adds the ability to add underlining to the Typography for web components.

This is accomplished by adding an underline prop to the Typography component, which is a space separated string consisting of a style and optionally, a color. The value for style can be set to one of the four allowed values, whereas the color can either be omitted or be set to one of the color values from our color tokens. If the color is omitted from the underline prop, it defaults to the text's color itself (whether provided via textColor or inherited naturally via CSS).

Changes

Type Safety for Prop Full type safety and autocomplete is provided for the new prop:

Browsers This is how the functionality looks in Storybook in the three major browsers:

Firefox
Chrome
Safari

Props Here are some different variations of the props, changing the style of the underline, the color of the text and when setting and style and color of text:

Our in app changes using this

Details

Apply the following diff to see this: ```diff diff --git a/app/javascript/jobber/jobberAssistant/components/ConversationListItem.module.css b/app/javascript/jobber/jobberAssistant/components/ConversationListItem.module.css index 7e5e5911b5b..4b2d54081f7 100644 --- a/app/javascript/jobber/jobberAssistant/components/ConversationListItem.module.css +++ b/app/javascript/jobber/jobberAssistant/components/ConversationListItem.module.css @@ -24,6 +24,7 @@ .responseContainer { display: flex; + flex-wrap: wrap; box-sizing: border-box; padding: 0; border: none; @@ -41,6 +42,7 @@ display: flex; min-width: 0; margin-bottom: 0; + margin-left: 32px; text-align: left; flex-direction: column; gap: var(--space-base); diff --git a/app/javascript/jobber/jobberAssistant/components/ConversationListItem.tsx b/app/javascript/jobber/jobberAssistant/components/ConversationListItem.tsx index ad062fa1c9d..1589d2c0851 100644 --- a/app/javascript/jobber/jobberAssistant/components/ConversationListItem.tsx +++ b/app/javascript/jobber/jobberAssistant/components/ConversationListItem.tsx @@ -4,7 +4,7 @@ import { Icon } from "@jobber/components/Icon"; import { Text } from "@jobber/components/Text"; import { Emphasis } from "@jobber/components/Emphasis"; import classNames from "classnames"; -import { Markdown } from "@jobber/components"; +import { Markdown, Tooltip } from "@jobber/components"; import type { AiAssistantAction, AiAssistantFollowup, @@ -119,6 +119,13 @@ const Response = ({ item, showFeedbackOptions }: ConversationListItemProps) => {

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

Deploying atlantis with    Cloudflare Pages

Latest commit:	7efb402
Status:	✅  Deploy successful!
Preview URL:	https://a0a0c5ec.atlantis.pages.dev
Branch Preview URL:	https://job-104497-add-underlining-t.atlantis.pages.dev

View logs

[ZakaryH]

something I'm still on the fence about, which I believe is the reason we didn't have underline to begin with, is the overlap between underlined text and Links.

both will look almost identical, and you won't be able to tell if something is a link by looking at it. for example, even here on Github you can tell these are links

however in our system, they could be underlined blue Typography

after taking a quick peek in product, the vast majority of places we are adding text-decoration: underline is on links

[ZakaryH]

how might this work with Text/Typography that isn't just a single sentence?

imagine I only want this part of a sentence to be underlined

edit: ah, I forgot you can specify what element to render Typography as, so you'd make it a span and that should work

Text doesn't offer that, but I suppose that's fine. there's quite a few things like that now.

does look a bit cramped between the underlined and not. ah, that's because my editor is removing the space at the end of the lines. if I tell it to keep them, the spacing is fine.

[sabidhasan]

something I'm still on the fence about, which I believe is the reason we didn't have underline to begin with, is the overlap between underlined text and Links.

both will look almost identical, and you won't be able to tell if something is a link by looking at it. for example, even here on Github you can tell these are links

however in our system, they could be underlined blue Typography

after taking a quick peek in product, the vast majority of places we are adding text-decoration: underline is on links

@ZakaryH this is a good question, and I will tag @jlelford to provide some of the design impetus/motivations behind this change because some thought and discussion has been been put into it.

how might this work with Text/Typography that isn't just a single sentence?

Wouldn't this already be the case for all of the other props to Text as it is? For instance, how could I only italicize a part of the text? Or highlight a part of it? Or bold a part of it? Or change the color for a part?

The way I am thinking about it is that Text is a semantic equivalent for the HTML p element. Since one can't underline a part of p, we also shouldn't support underlining a part of Text.

Edit: the above is for Text, for Typography, what you said :)

[chris-at-jobber]

Related: Emphasis is typically what we use to add stylistic emphasis to a portions of a piece of text

[jlelford]

something I'm still on the fence about, which I believe is the reason we didn't have underline to begin with, is the overlap between underlined text and Links.

@ZakaryH the concern between straight underline and links is warranted, but what I really wanted as a tool is the "dotted" version of it, which does have associations with presenting tooltip like information in other design systems. It is a method for us to display interactivity within text content that doesn't imply actual navigation.

I'm not sure we have a need for the "other variants" of underline that aren't covered by the existing use of links. Is there something you or @chris-at-jobber would recommend us do to allow us to have the desired effect of the dotted text without opening the door on regular underlines diluting text links?

[ZakaryH]

that makes sense having the dotted version to indicate interactivity, though if that's our main priority - that's the one thing we don't have available on Android. are you guys considering following up with adding that at all?

keeping the APIs between mobile and web as consistent as possible is something we're trying to do, so at this point I think we continue adding underline to Text/Typography in web

if that wasn't already in place I think I probably would have thought about maybe Emphasis having an interactive variant or something along those lines since I'm really not seeing a need for Text/Typography to support underlines outside of links.

though Emphasis doesn't exist in mobile at all, and we aren't currently able to combine multiple Text/Typography components so that would involved a bit more work to enable that.

[sabidhasan]

Related: Emphasis is typically what we use to add stylistic emphasis to a portions of a piece of text

@chris-at-jobber - would it be better to have a new variation for emphasis for dottedUnderline (name pending of course) or something like that? That approach might address your concerns in constraining the scope of the underlining possible, and address Zak's concerns around links looking like underlines, since only.

Though this approach does open up an issue around the color of the underline, where @jlelford 's intention was to have a different color for the underline vs the text. It also creates a discrepancy between the way underlining is done on mobile and desktop (per the other PR we released last week)

[ZakaryH]

I know it's a bit late, since we've already committed to this via the mobile implementation but I'm having doubts about adding it to Text.

Typography sure - that one has tons of customizable props

looking at the rest of the props for Text, it feels out of place. also if Text is more like a paragraph, is it reasonable to have an entire paragraph underlined? I worry about readability.

what would be your use case for having underlined Text?

[ZakaryH]

actually, I don't think it's too late. the odds of someone else using that prop in the past few days is exceedingly low.

talked with Chris, and what we'd like to do is:

• revert the change to Text in components-native, so the prop no longer exists on Text
• have your usage in mobile swap to use Typography instead
• do the same thing in web components, where only Typography implements it

what do you think about that? I see we're currently using Text in mobile for the instance we needed last week. is there any reason Typography wouldn't work there?

[sabidhasan]

actually, I don't think it's too late. the odds of someone else using that prop in the past few days is exceedingly low.

talked with Chris, and what we'd like to do is:

• revert the change to Text in components-native, so the prop no longer exists on Text
• have your usage in mobile swap to use Typography instead
• do the same thing in web components, where only Typography implements it

what do you think about that? I see we're currently using Text in mobile for the instance we needed last week. is there any reason Typography wouldn't work there?

I am OK with that approach - so to confirm: no change goes into Text, and we only make changes in Typography.

For mobile, what priority would you prefer on this work? We already have the mobile cleanup jira ticket and we can do it as part of that, or I can make the change now to introduce revert the Text changes.

In this PR, once I make the change to remove any changes to Text, we should be good?

[sabidhasan]

@ZakaryH - made the change in this PR to revert changes to Typography in 7efb402986f3191c505c86134407f2d34bf4b3ae. I think this should address the concerns for this PR. For the mobile work, we can create a separate PR to scope the changes to mobile - we can either do this as a fast follow to this PR, or if you don't think this is high priority then I can do it when we pull in the aforementioned ticket. 🙏🏾

[ZakaryH]

I am OK with that approach - so to confirm: no change goes into Text, and we only make changes in Typography.

correct. it would only be exposed by Typography, which is the component that we allow all sorts of customization on vs Text which is pretty locked down with minimal options for manipulating the appearance outside of a few presets

For mobile, what priority would you prefer on this work? We already have the mobile cleanup jira ticket and we can do it as part of that, or I can make the change now to introduce revert the Text changes.

I suppose it depends how soon you're thinking you'll be on the follow up work? be great to have the revert sooner than later to minimize the possibility someone starts using the prop on Text in mobile.

I'd say medium priority. the odds of someone using the prop are pretty low, but it would technically be a breaking change so keeping the window of opportunity as small as we can is preferable.

In this PR, once I make the change to remove any changes to Text, we should be good?

yup! I'll give it one quick read over, but I can't think of anything else pressing.