Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isom-1727 bug external link all should have new tab icon open in new #1139

Conversation

adriangohjw
Copy link
Contributor

Problem

For buttons and collection/blog cards, external links 1) do not open in new tab and 2) show "new tab" icon at the end

image

Closes https://linear.app/ogp/issue/ISOM-1727/bug-external-link-all-should-have-new-tab-icon-open-in-new-tab

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Bug Fixes:

  • add a check in LinkButton
  • add a check in the CollectionCard and BlogCard + add stories

Tests

  1. go to a component with a button e.g. Hero. Add the link to an external link e.g. https://google.com.
    • It should show a "open in new link" icon
    • after building, when clicked on, it should open new link. (alternately: can inspect element on studio to check for target: _blank
  2. go to a collection and create a collection link. Add the link to an external link e.g. https://google.com.
    • It should show a "open in new link" icon
    • after building, when clicked on, it should open new link. (alternately: can inspect element on studio to check for target: _blank

@adriangohjw adriangohjw added the bug Something isn't working label Feb 24, 2025
@adriangohjw adriangohjw self-assigned this Feb 24, 2025
@adriangohjw adriangohjw requested a review from a team as a code owner February 24, 2025 05:02
Copy link

linear bot commented Feb 24, 2025

@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Feb 24, 2025

Datadog Report

Branch report: adriangohjw/isom-1727-bug-external-link-all-should-have-new-tab-icon-open-in-new
Commit report: ca03a6d
Test service: isomer-studio

✅ 0 Failed, 290 Passed, 37 Skipped, 55.07s Total Time
🔻 Test Sessions change in coverage: 1 decreased (-0.11%)

🔻 Code Coverage Decreases vs Default Branch (1)

  • vitest run --coverage 7.43% (-0.11%) - Details

}),
}

// TODO: fix the external link icon showing up when the text is long
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't understand this comment, what's the current behaviour when the text is long?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, should have been clearer

current expected
image image

couldn't find a quick easy way to do this so leaving it as a TODO - esp. since not that common for titles to be so long

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move the line-clamp-3 to the text span specifically, but the icon will be on the next line

image.png

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was hesistating on this too because it's not the most ideal, tho you might be right that it's better-than-nothing. will make the UI change, but still keep the commment to explain the desired outcome!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cc @sehyunidaaa fyi

Copy link
Contributor Author

@adriangohjw adriangohjw Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realized it's not as simple to apply line-clamp-3 to the text span as that would mean the icon is always on the next line, which isn't ideal when the title is too short and not truncated.

instead, updated it (e1fdb2d, 4cf732c, f0e2e9d) such that

  • there's a useEffect to check if the text is truncated before deciding the location of the icon
  • and because it's a useEffect, we will need use client so i further split it into InternalLinkTitle (rendered during SSG) and externalLinkTitle (rendered during client side)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I check this on storybook? Tried on this, but still was unable to see the icon

image.png

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -1,5 +1,4 @@
import type { Meta, StoryObj } from "@storybook/react"
import { expect, userEvent, within } from "@storybook/test"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR - just saw that it's not actually being used

Comment on lines +24 to +44
interface GenerateArgsProps {
shouldShowDate?: boolean
isLastUpdatedUndefined?: boolean
withoutImage?: boolean
title?: string
description?: string
tags?: Tag[]
isExternalLink?: boolean
}

const generateArgs = ({
shouldShowDate = true,
isLastUpdatedUndefined = false,
withoutImage = false,
title = "A journal on microscopic plastic and their correlation to the number of staycations enjoyed per millennials between the ages of 30-42, substantiated by research from IDK university",
description = "We've looked at how people's spending correlates with how much microscopic plastic they consumed over the year. We've looked at how people's spending correlates with how much microscopic plastic they consumed over the year.",
tags = [],
}: {
isExternalLink = false,
}: GenerateArgsProps): Partial<CollectionCardProps> & {
shouldShowDate?: boolean
isLastUpdatedUndefined?: boolean
withoutImage?: boolean
title?: string
description?: string
tags?: Tag[]
}): Partial<CollectionCardProps> & { shouldShowDate?: boolean } => {
} => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight refactoring here since props is getting larger

Comment on lines +24 to +43
interface GenerateArgsProps {
shouldShowDate?: boolean
isLastUpdatedUndefined?: boolean
withoutImage?: boolean
title?: string
description?: string
tags?: Tag[]
isExternalLink?: boolean
}
const generateArgs = ({
shouldShowDate = true,
isLastUpdatedUndefined = false,
withoutImage = false,
title = "A journal on microscopic plastic and their correlation to the number of staycations enjoyed per millennials between the ages of 30-42, substantiated by research from IDK university",
description = "We've looked at how people's spending correlates with how much microscopic plastic they consumed over the year. We've looked at how people's spending correlates with how much microscopic plastic they consumed over the year.",
tags = [],
}: {
isExternalLink = false,
}: GenerateArgsProps): Partial<CollectionCardProps> & {
shouldShowDate?: boolean
isLastUpdatedUndefined?: boolean
withoutImage?: boolean
title?: string
description?: string
tags?: Tag[]
}): Partial<CollectionCardProps> & { shouldShowDate?: boolean } => {
} => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight refactoring here since props is getting larger

@adriangohjw
Copy link
Contributor Author

note - changes from d02f2f4 and 156b7f7 are such that the default UI in most storybook will not have the external link icon

@adriangohjw adriangohjw merged commit b901e38 into main Feb 26, 2025
20 checks passed
@adriangohjw adriangohjw deleted the adriangohjw/isom-1727-bug-external-link-all-should-have-new-tab-icon-open-in-new branch February 26, 2025 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants