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

Navlink merch store target fix #1848

Merged

Conversation

franciscusagnew
Copy link
Contributor

@franciscusagnew franciscusagnew commented Jan 26, 2025

Description of changes

This PR fixes a minor routing issue with the Merch Store navigation link where an outside link opens in the current tab instead of a new tab as recommended and as it does in the “Get Involved” page.

Issue Resolved

Fixes #NA

Screenshots/GIFs

BEFORE:
Merch Store

Opens in the current browser tab
Merch Store 2

AFTER:
Opens in a new browser tab
Merch Store 3

Copy link

vercel bot commented Jan 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
operation-code ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 5:06pm
storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 5:06pm

Copy link

cypress bot commented Jan 26, 2025

operation_code    Run #4821

Run Properties:  status check passed Passed #4821  •  git commit 038398f1a5: Cleanup for Nav.tsx & NavListItem.tsx
Project operation_code
Branch Review pull/1848
Run status status check passed Passed #4821
Run duration 03m 35s
Commit git commit 038398f1a5: Cleanup for Nav.tsx & NavListItem.tsx
Committer Franciscus Agnew
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 25
View all changes introduced in this branch ↗︎

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

Instead of this PR, I'd rather OutboundLink be useable inside Nav for the one Merch Store link. It adds exit analytics and "opens in new tab" iconography.

@kylemh
Copy link
Member

kylemh commented Jan 28, 2025

looks like diff padding/margin between the items.
Screenshot 2025-01-28 at 6 54 40 PM

also, it's missing the iconography used in other locations. i think hasIcon needs to be true.
Screenshot 2025-01-28 at 6 56 19 PM

@franciscusagnew
Copy link
Contributor Author

franciscusagnew commented Jan 28, 2025

@kylemh: Yes, I observed that as well. I wanted to confirm the implementation of the OutboundLink functionality as requested within the navigation before making any visual changes. I wasn't certain whether or not to include the icon and set it to false, thanks for the clarifications. I believe we got it all covered now!

@kylemh
Copy link
Member

kylemh commented Jan 28, 2025

Ah. Functionally, it's perfect!

components/Nav/Nav.tsx Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Jan 31, 2025

Code Climate has analyzed commit 038398f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 82.6% (90% is the threshold).

This pull request will bring the total coverage in the repository to 81.1% (0.0% change).

View more on Code Climate.

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

perfect

@kylemh kylemh merged commit e21daba into OperationCode:main Jan 31, 2025
13 of 14 checks passed
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.

2 participants