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

app-project: Refactor NavLink to handle internal vs external route #6320

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Sep 18, 2024

Package

app-project

Linked Issue and/or Talk Post

Related to #6318
Links to external routes should not use next/link. PFE pages are considered external routes as well as fe-root. They're all completely separate web apps.

Describe your changes

  • Removed disabled state from NavLink. This prop is not used anywhere in combo with NavLink in app-project. There's no reason to keep it, and much more elegant solutions exist to display a disabled link or button as needed such as a disabled PlainButton).
  • Added externalLink option to the link prop passed to shared component NavLInk. Rather than rip out NavLink from app-project components that shouldn't be using a next/link (there's a lot of them 😬 ), I refactored NavLink with logic to handle whether to use next/link depending on:
    link = {
      externalLink: true,
      href: 'etc',
      text: 'etc'
    }
    
    • I also chose this solution because app-project has a shared component called ContentBox that can accept linkProps which get passed on to NavLink. I only found one case where this was actually happening (project stats), but in order to not disrupt the one-size-fits-all ContentBox behavior, I chose to adapt NavLink's code.

How to Review

This is best tested by running locally in production mode:

  • Run any project locally and check that the ProjectHeader links do client-side routing for project home, about, and classify. You'll see prefetch requests logged to the terminal when hovering over ProjectTitle (link to homepage), or About or Classify. Same on any project's About pages, prefetching happens on all of the links in the About Project sidebar.
  • On any project's Team page, the username should still form the link-to-user correctly.
  • Link to a project's stats page should also be formed correctly.

(You will still see one failed prefetch for /talk in the network tab because of the JoinInButton handled in #6318).

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

@coveralls
Copy link

coveralls commented Sep 18, 2024

Coverage Status

coverage: 78.787% (-0.008%) from 78.795%
when pulling 840be25 on app-project-navlink-refactor
into ffaab1e on master.

@goplayoutside3 goplayoutside3 requested a review from a team September 18, 2024 19:45
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