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.

Sign up for GitHub

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

Jump to bottom

feat: [InstitutionHeading] Support rendering as a link (isProfileLink) to the institution profile #1021

Closed
wants to merge 3 commits into from
Closed

feat: [InstitutionHeading] Support rendering as a link (isProfileLink) to the institution profile #1021

wants to merge 3 commits into from

Conversation

meissadia
Copy link
Contributor

@meissadia meissadia commented Oct 18, 2024 •
edited
Loading

Closes #1012

Changes

  • feat: InstitutionHeading: Added prop isProfileLink to determine if the heading should render as a link to the Institution profile
  • task: Filing app pages that use InstitutionHeading: Provide LEI so that we can link to the Institution profile

How to test this PR

  1. Run through the filing process
  2. On each page of the process, confirm the Institution heading's link redirects to the Institution profile

Screenshots

Unvisited Visited
Screenshot 2024-10-18 at 11 12 06 AM Screenshot 2024-10-18 at 11 12 17 AM

billhimmelsbach
billhimmelsbach reviewed Oct 21, 2024
View reviewed changes
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.

I think we'll need to underline these institution links for accessibility? @natalia-fitzgerald

@natalia-fitzgerald
Copy link

@meissadia
Thanks for sharing the screenshots. This has helped to clarify some additional aspects of changing this to a link that I hadn't yet considered. I agree that ideally we would add an underline for accessibility and link consistency BUT adding the blue color and particularly the underline will affect the design. There's already an underline line (horizontal line) balancing act going on.

Can we pause on this work of making the financial institution name clickable? I need to think through the addition of the underline. And, since this work isn't tied to our sprint goals I'd like to take some time to discuss it with UX.

@meissadia meissadia marked this pull request as draft October 22, 2024 16:15
@meissadia meissadia mentioned this pull request Oct 29, 2024
Open
@meissadia meissadia closed this Oct 29, 2024
@meissadia
Copy link
Contributor Author

Closing PR but keeping the unmerged branch as a future reference for #1012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billhimmelsbach billhimmelsbach billhimmelsbach left review comments

@shindigira shindigira Awaiting requested review from shindigira

@ojbravo ojbravo Awaiting requested review from ojbravo

@tanner-ricks tanner-ricks Awaiting requested review from tanner-ricks tanner-ricks is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Filing] Institution name links to Institution profile
3 participants
@meissadia @natalia-fitzgerald @billhimmelsbach