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

[Links] Show external-link icon where applicable #768

Merged
merged 21 commits into from
Oct 28, 2024
Merged

Conversation

meissadia
Copy link
Collaborator

@meissadia meissadia commented Jun 25, 2024

Close #765

Changes

  • feat: [Link] Programmatically determine internal vs external links and add icon where appropriate
    • Links to specific iRegs open in new tab
    • Links to specific FIG sections open in new tab
    • mailto: links are treated as internal
  • task: Ensure all instances of <Link> use the SBL Link for consistent styling and functionality
  • task: [Link] Refactor to simplify implied usage of RouterLink
  • task: [Link] Remove unnecessary usages of the isRouterLink prop
  • task: Overrides DS Notification styles to ensure icon colors consistently match link's status-based (visited, hover) styles

Screenshots

Filing home

screencapture-localhost-8899-2024-10-23-11_44_51

Filing landing

shared-landing

Filing overview

screencapture-localhost-8899-filing-2024-10-23-11_39_04

Filing upload

screencapture-localhost-8899-filing-2024-123456789TESTBANK123-upload-2024-10-23-11_40_46

Filing errors

screencapture-localhost-8899-filing-2024-123456789TESTBANK123-errors-errors-syntax-2024-10-23-11_41_19

Filing warnings

screencapture-localhost-8899-filing-2024-123456789TESTBANK123-warnings-2024-10-23-11_41_53

Filing Point of Contact

screencapture-localhost-8899-filing-2024-123456789TESTBANK123-contact-2024-10-23-11_42_40

Filing Sign and Submit

screencapture-localhost-8899-filing-2024-123456789TESTBANK123-submit-2024-10-23-11_43_24

User profile

screencapture-localhost-8899-profile-view-2024-10-23-11_44_19

@meissadia
Copy link
Collaborator Author

meissadia commented Jun 28, 2024

@natalia-fitzgerald Please follow-up with devs on how we want to handle external links where the icon wraps to a new line.

One potential solution is to use the Link component's noWrap property, which would keep the link text + icon together, but looks like it could introduce it's own wrapping oddities. Example screenshot below:

Description Screenshot
Without noWrap (allow link text to wrap) Screenshot 2024-06-28 at 2 47 16 PM
With noWrap (prevent link text from wrapping) Screenshot 2024-06-28 at 12 36 33 PM

@natalia-fitzgerald
Copy link

@meissadia
Can you show the same two screenshots at mobile? I do worry that forcing no wrap could introduce even further oddities at smaller screen widths.

@meissadia
Copy link
Collaborator Author

@meissadia Can you show the same two screenshots at mobile? I do worry that forcing no wrap could introduce even further oddities at smaller screen widths.

@natalia-fitzgerald On large mobile (width: 425px)

Regular No wrap
regular nowrap

@natalia-fitzgerald
Copy link

@meissadia
Would it be possible to push the no wrap solution to a dev server so that I can click around and see how that approach looks across a few instances?

@meissadia
Copy link
Collaborator Author

Implemented no-wrap for external links, ready for deploy to DEV for evaluation by Design.

@billhimmelsbach
Copy link
Contributor

Going to start deploying this now!

@billhimmelsbach
Copy link
Contributor

It's up on Dev @meissadia and @natalia-fitzgerald!

@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Oct 18, 2024

@meissadia
I took a look at the no-wrap implementation on Dev.

Assessment of no wrap

After additional consideration, I recommend that we handle external links the way we handle any link + icon. My preference is to follow the CFPB Design System and cf.gov website approach to link + icon, unless we identify a problem with this general approach. I believe that the standard approach in the DS and on cf.gov is to allow wrapping of links with and without icons. Is that your understanding? Here's a link to the link + icon section in the CFPB Design System.

One odd line break I saw was for Global LEI Foundation (GLEIF) where the no wrap made this stay on one line instead of wrapping and created awkward white space. Here the wrap solution is preferable.

Regular No wrap
Screenshot 2024-10-17 at 8 52 50 PM Screenshot 2024-10-17 at 8 52 58 PM

When does an external link icon appear next to a link?

On cf.gov, we use the external links icon for links to pages that take you outside of the CFPB sphere of webpages, examples on the beta platform would be Login.gov, the Federal Reserve, GLEIF, etc. In terms of what's shown on Dev it seems like the external link is being added to any link any link that will take you outside of the beta platform, even links to cf.gov content, for example the filing instructions guide and iRegs. If we go with the approach of only including the external link icon for links that take you out of the CFPB sphere of webpages, the number of instances would be greatly reduced, and this task would be a lot narrower. I recommend we take that approach.

I noticed some odd outlier behaviors that we should address:

Dotted underline extending under the icon
Dotted underline should not extend beyond the link text.

Underline is extending under the icon
Screenshot 2024-10-17 at 8 22 55 PM

Dotted underline is missing
All standard inline links and list links should include the dotted underline.

Underline is missing
Screenshot 2024-10-17 at 8 18 39 PM

Links to the same pages sometimes have an external link icon and sometimes don't.
The use of the external link icon should be consistently applied.

Filing instructions guide - no icon Filing instructions guide - includes icon
Screenshot 2024-10-17 at 8 41 38 PM Screenshot 2024-10-17 at 8 41 33 PM
Email our support staff includes icon and then doesn't
Screenshot 2024-10-17 at 8 59 07 PM

@meissadia
Copy link
Collaborator Author

meissadia commented Oct 18, 2024

@natalia-fitzgerald Thanks you very much for the detailed review!! This is phenomenal guidance for my next steps.

  • No wrap
    • Yes, DS Links wrap even if icon ends up on a separate line than the link text. I will follow that guidance.
  • CPFB sphere
    • I will work to compile a list of "internal" domains so that we can programmatically derive internal vs external
    • I imagine we want this functionality implemented in the DSR so that it is consistent across all CFPB applications
  • Inconsistencies I am working to address
    • Dotted underline should not extend beyond the link text
    • Dotted underline is missing
    • The use of the external link icon should be consistently applied

tanner-ricks and others added 7 commits October 21, 2024 13:19
Fixed a bug that was causing one space to appear instead of the intended
two.

## Changes

- src
  - pages
    - Filing
      - FilingApp
- InsitutionHeading.tsx: Replaced double spaces with double unicode
non-breaking spaces.

## How to test this PR

1. Pull the branch
2. Relaunch whatever is necessary
3. Navigate to a page in the app that contains the Filing Steps and the
Institution Heading
4. Verify that the pipe character is surrounded by two spaces in the
Institution Heading. The attached screenshot has a red box which
surrounds the area to check.

## Screenshots
![Screenshot 2024-10-17 at 12 27
18 PM](https://github.com/user-attachments/assets/06b50ca5-76fb-4920-849c-47e67e5b1537)
…#1001)

closes #997

## Changes

- style(Point of Contact): Phone Extension always gets its own line
- content(Point of Contact): Change the helper and error text to match
the figma
- feat(Point of Contact): Phone Extension validation


## How to test this PR

1. Navigate to Point of Contact
2. Enter valid phone extension - ex. 00757
3. Enter invalid phone extension - ex ukhi778ih89h98h9h9h89h

## Screenshots
<img width="534" alt="Screenshot 2024-10-15 at 3 08 40 PM"
src="https://github.com/user-attachments/assets/7ed7bae8-9bc0-489f-9e63-a7748e8b9582">

---------

Co-authored-by: Tanner Ricks <182143365+tanner-ricks@users.noreply.github.com>
task: [Link] Refactor to simplify implied usage of RouterLink
@meissadia meissadia marked this pull request as ready for review October 21, 2024 19:37
task: Link - Consider `mailto:` links as internal
Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Heyo! A couple changes needed here.

src/components/Link.utils.tsx Outdated Show resolved Hide resolved
src/components/Link.utils.tsx Outdated Show resolved Hide resolved
src/components/Link.utils.tsx Outdated Show resolved Hide resolved
@meissadia meissadia marked this pull request as draft October 23, 2024 18:44
Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Heyo! Just summarizing our review from yesterday @meissadia:

  1. Let's keep the underline for emails (like on the authenticated landing page), maybe take a quick look at why the underline is removed there when it's considered internal now?
  2. Add an exception to the _blank logic for iRegs links and filing instruction guides, maybe based on the url?

We'll handle more external link logic at a later date.

@meissadia meissadia marked this pull request as ready for review October 24, 2024 21:53
@meissadia meissadia requested review from billhimmelsbach and removed request for ojbravo and shindigira October 24, 2024 21:54
Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@meissadia
The external links icon work looks good on dev pub. We plan to create a more logical system for when links should and shouldn't open in a new tab so I'm fine with the in between implementation since we'll iterate on this soon. Here's the spike issue where Dan and I are discussing this behavior. cfpb/sbl-ux#18

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Code looks good, thanks for the fixes!

@meissadia meissadia merged commit 267d021 into main Oct 28, 2024
4 checks passed
@meissadia meissadia deleted the external-link-icon branch October 28, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Links] Show external link icon where applicable
5 participants