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

Migrate doi pages to app router #342

Merged
merged 21 commits into from
Feb 29, 2024
Merged

Migrate doi pages to app router #342

merged 21 commits into from
Feb 29, 2024

Conversation

bklaing2
Copy link
Member

@bklaing2 bklaing2 commented Feb 7, 2024 •
edited
Loading

Purpose

Migrate /doi.org pages to the app router

closes: #333

Approach

I followed a similar approach to the index page migration

Open Questions and Pre-Merge TODOs

  • The most significant changes are in the src/app/doi folder.
    • Most of the other changes are due to imports changing to the new types.ts file and react-bootstrap wrappers, or adding the 'use client' directive
  • When building the project, the CSS imports get mixed up. I moved the doi.css import from the root layout page into the styles.css file
  • Some cypress tests were throwing errors that don't appear when I manually follow the same steps. After spending some time looking into it with no success, I decided to disable them for the time being
    • This was resolved by
      • setting experimentalFetchPolyfill: false in cypress.config.ts
      • updating the GitHub action to test against the production build instead of the dev build
      • Telling Cypress to ignore hydration errors because Cypress and React Hydration don't get along
  • Some react-bootstrap interactions (like the Tabs buttons on the DOI page) don't work when running yarn run dev. However, they work with yarn run build

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@bklaing2 bklaing2 requested a review from jrhoads February 7, 2024 15:34
@bklaing2 bklaing2 self-assigned this Feb 7, 2024
@cypress Cypress
Copy link

cypress bot commented Feb 7, 2024 •
edited
Loading

Passing run #1075 ↗︎

0 56 0 0 Flakiness 0

Details:

Merge 2f07793 into 27f2325...
Project: akita Commit: 299df21661 ℹ️
Status: Passed Duration: 03:43 💡
Started: Feb 29, 2024 9:56 AM Ended: Feb 29, 2024 10:00 AM

Review all test suite changes for PR #342 ↗︎

@cypress Cypress
Copy link

cypress bot commented Feb 8, 2024

3 flaky tests on run #1051 ↗︎

0 55 0 0 Flakiness 3

Details:

Fixed failing tests
Project: akita Commit: 36216833af
Status: Passed Duration: 02:56 💡
Started: Feb 8, 2024 3:32 PM Ended: Feb 8, 2024 3:35 PM
Flakiness  search.test.ts • 1 flaky test • Tests

View Output

Test Artifacts
... > search with enter Test Replay Screenshots
Flakiness  personContainer.test.ts • 1 flaky test • Tests

View Output

Test Artifacts
PersonContainer > identifiers Test Replay Screenshots
Flakiness  statistics.test.ts • 1 flaky test • Tests

View Output

Test Artifacts
Overview > header Test Replay Screenshots

Review all test suite changes for PR #342 ↗︎

@bklaing2 bklaing2 marked this pull request as ready for review February 9, 2024 10:35
jrhoads
jrhoads reviewed Feb 9, 2024
View reviewed changes
src/app/layout.tsx Outdated Show resolved Hide resolved
jrhoads
jrhoads reviewed Feb 9, 2024
View reviewed changes
src/app/not-found.tsx Outdated Show resolved Hide resolved
jrhoads
jrhoads reviewed Feb 9, 2024
View reviewed changes
src/app/doi.org/[...doi]/page.tsx Outdated
if (!data) return notFound()


// return <script type="application/ld+json">{work.schemaOrg}</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of place here

Copy link
Member Author

@bklaing2 bklaing2 Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This originates from here. The app router doesn't like having scripts in the <Head> like we had in the pages router, but I didn't want delete it entirely

jrhoads
jrhoads reviewed Feb 9, 2024
View reviewed changes
src/app/doi.org/page.tsx Outdated Show resolved Hide resolved
jrhoads
jrhoads approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@jrhoads jrhoads left a comment

Choose a reason for hiding this comment

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

I acknowledge we have commented out the consent tests.
Our plan is to re-enable them in the near future.

@bklaing2 bklaing2 merged commit a5b0631 into master Feb 29, 2024
11 checks passed
@bklaing2 bklaing2 deleted the migrate-doi-pages-to-app-router branch February 29, 2024 10:07
@jrhoads jrhoads mentioned this pull request Mar 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhoads jrhoads jrhoads approved these changes

Assignees

@bklaing2 bklaing2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate DOI pages to app router
2 participants
@bklaing2 @jrhoads