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

Fix Finished Announcement banner results link #6373

Closed
wants to merge 2 commits into from

Conversation

shaunanoordin
Copy link
Member

PR Overview

Package: app-project
Fixes #6370

This PR fixes the Finished Announcement banner's "See Results" link.

  • Prior to this fix, the "See Results" link failed to add a "/projects" to the URL.
  • Storybook Story has been fixed: 'baseUrl' now matches the 'baseUrl' returned by the Project store (which doesn't automagically prepend a "/projects" to the project slug).
  • Tests have been fixed: prior to this, the tests would always return incorrect passes due to the use of ".findBy..." returning a Promise that always .exists().

Testing

Test on app-project, on localhost

Status

Ready for review.

Note: if this gets approved while I'm on vacation, please go ahead and merge+deploy this.

@shaunanoordin shaunanoordin requested a review from a team October 10, 2024 23:44
@shaunanoordin shaunanoordin added the bug Something isn't working label Oct 10, 2024
@coveralls
Copy link

Coverage Status

coverage: 78.576% (+0.03%) from 78.542%
when pulling 9f0754d on fix-finishedannouncement-results-link
into 4062f39 on master.

@@ -23,7 +23,7 @@ function FinishedAnnouncementConnector() {

const announcement = t('Announcements.FinishedAnnouncement.announcement')
const link = {
href: `${baseUrl}/about/results`,
href: `/projects${baseUrl}/about/results`,
Copy link
Contributor

@eatyourgreens eatyourgreens Oct 11, 2024

Choose a reason for hiding this comment

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

This link should be using the Next router, which automatically adds the app base path.

If the link doesn’t use the client-side router, you lose all project state when you follow it, including classifier state.

Copy link
Contributor

Choose a reason for hiding this comment

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

The base path for internal routing, within the Next app, is set here:

Copy link
Contributor

@eatyourgreens eatyourgreens Oct 12, 2024

Choose a reason for hiding this comment

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

#6374 fixes the underlying bug in the NavLink component, which was causing this link to be rendered as an external link.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Oct 11, 2024

I've opened #6374 to fix the underlying bug in the NavLink component, which is triggered when you use the default StyledAnchor prop. The NavLink code fails if the Grommet Anchor isn't wrapped in styled(), and renders it as a bare link.

Project navigation links pass a StyledAnchor prop with the navigation styles, as does the 'Learn more' link on the project home page, so they render correctly. The Finished banner doesn't pass that prop, so triggers this bug.

Here's an example of a working internal link, from the project introduction. This is the Learn More link.

<NavLink
gap='xsmall'
icon={<Next color='light-5' size='12px' />}
link={link}
reverse
StyledAnchor={StyledAnchor}
/>

const link = screen.findByLabelText('Announcements.FinishedAnnouncement.seeResults')
expect(link).exists()
const link = screen.getByRole('link')
expect(link?.getAttribute('href')).to.equal('/projects/zookeeper/galaxy-zoo/about/results')
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests don't use next.config.js, which is why the app base path isn't included in these link URLs.

@@ -22,7 +22,7 @@ function NextRouterStory(Story) {

const mockStore = {
project: {
baseUrl: '/projects/zookeeper/galaxy-zoo',
baseUrl: '/zookeeper/galaxy-zoo',
Copy link
Contributor

Choose a reason for hiding this comment

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

These links have /projects prepended because the storybook doesn't get the app base path from next.config.js, so it has to be added by hand.

@goplayoutside3 goplayoutside3 self-assigned this Oct 16, 2024
@goplayoutside3
Copy link
Contributor

@shaunanoordin I took a look at both this PR and #6374, and I'm going to merge the latter. It fixes the broken results link and future proofs any other internal links that might've left out StyledAnchor.

Thank you for documenting the findBy tests in #6383 too!

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.

Finished Announcement banner links to invalid Results page
4 participants