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

Convert 'contact', 'team', and 'whoami' to app router; partially convert '404' [#134] #1032

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

genehack
Copy link
Contributor

@genehack genehack commented Sep 27, 2024

Description of proposed changes

This converts the global/shared layout and navigation bits, the /contact, /team, and /whoami pages (including login state display in that page and the global nav), and the general outline of the 404/not-found page over to using the App Router. The 404 page has been changed to provide a link to the root page, rather than loading the (as-yet-unported) <Splash> component.

This also required converting all styled-components usage in those parts over to either regular CSS, or to CSS Modules, depending on the scope of the usage. In general, I have tried to keep things scoped either very tightly (i.e., in specific CSS Modules imported into a single component) or put them in a global CSS file that is imported in the root layout.

I have elected to migrate components to /static-site/components as they are converted over for use with App Router; that felt like the cleanest option. I also elected to put page content directly into the app/*/pages.tsx files, rather than propagating the "page content is actually under static-site/src/pages/*.jsx" pattern, reasoning that this reduces the number of moving parts and makes it more clear where the content is relative to the the URL it is displayed at.

To be explicit: when this conversion work is done, I expect the contents of /static-site to be:

README.md
app/
components/
content/        ; maybe unify 
data/           ; these two?
next-env.d.ts
next.config.mjs
public/
static/         ; maybe unify with data?
tsconfig.json
types.d.ts
vendored/

Related issue(s)

#1052
#134

Checklist

@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-convert-bl-qibzaw September 27, 2024 19:54 Inactive
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from dea7987 to 163efe8 Compare September 27, 2024 20:03
@nextstrain-bot nextstrain-bot had a problem deploying to nextstrain-s-convert-bl-wpltic September 27, 2024 20:03 Failure
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 163efe8 to 91dede0 Compare September 27, 2024 20:04
@genehack genehack temporarily deployed to nextstrain-s-convert-bl-wpltic September 27, 2024 20:05 Inactive
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 91dede0 to 65263b2 Compare September 27, 2024 22:53
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-convert-bl-z0mywy September 27, 2024 22:53 Inactive
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 65263b2 to 1f5099a Compare September 27, 2024 22:57
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-convert-bl-cmum7s September 27, 2024 22:57 Inactive
@tsibley
Copy link
Member

tsibley commented Sep 30, 2024

Comparison via flicker:

before-after

@genehack
Copy link
Contributor Author

genehack commented Oct 1, 2024

Is that level of whitespace variation worth trying to nail down, IyourO?

@tsibley
Copy link
Member

tsibley commented Oct 1, 2024

Maybe not? Though if I were doing the work I'd probably still try. It also might have more impact on mobile viewports? Would be good to check. The logo position change seems like it'll have more impact when navigating between a page like the contact page and an Auspice page.

@genehack
Copy link
Contributor Author

genehack commented Oct 1, 2024

Maybe not? Though if I were doing the work I'd probably still try.

Oh believe me, there has been some previous effort in this direction, I assure you.

On first blush, I think I would push further effort getting the header alignment perfect to after adding the login/user tracking bit, as I suspect some of the delta is due to that.

It also might have more impact on mobile viewports? Would be good to check. The logo position change seems like it'll have more impact when navigating between a page like the contact page and an Auspice page.

Yeah, fair; I haven't done a lot of checking around mobile viewport sizes.

package.json Outdated Show resolved Hide resolved
@genehack
Copy link
Contributor Author

genehack commented Oct 1, 2024

BTW, what browser is this, and do you happen to remember the rough viewport size?

@genehack
Copy link
Contributor Author

genehack commented Oct 2, 2024

BTW, what browser is this, and do you happen to remember the rough viewport size?

@tsibley this was a question for you, about that comparison GIF — I'm having trouble reproducing some of the differences visible there with Firefox v131 on macOS.

@tsibley
Copy link
Member

tsibley commented Oct 3, 2024

BTW, what browser is this, and do you happen to remember the rough viewport size?

Firefox 113.0.2 on Linux (specifically 113.0.2+build1-0ubuntu0.18.04.1) with a viewport that's as wide as you see in the full size image (minus the red border).

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

I really like the direction this is heading, thank you for simplifying the styling ❤️

static-site/app/layout.tsx Outdated Show resolved Hide resolved
static-site/components/footer/index.tsx Outdated Show resolved Hide resolved
static-site/app/styles/bootstrap.css Show resolved Hide resolved
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 1f5099a to 8d1e000 Compare October 17, 2024 19:39
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 8d1e000 to 2104fe7 Compare October 17, 2024 23:12
@genehack genehack changed the title Convert 'contact' to app router [#134] Convert 'contact' and 'team' to app router [#134] Oct 17, 2024
@genehack
Copy link
Contributor Author

If folks want to take another look at this, I've addressed the feedback about the basic layout (I think! 🤞 ) and also finished the /team page port.

Heroku seems to be having some deployment issues at the moment; when a review app finally builds, I'll link a preview.

Next stage: dealing with the user context.

@victorlin victorlin temporarily deployed to nextstrain-s-convert-bl-98fca6 October 17, 2024 23:20 Inactive
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from 2104fe7 to b3216b1 Compare October 22, 2024 23:09
@genehack genehack temporarily deployed to nextstrain-s-convert-bl-98fca6 October 22, 2024 23:09 Inactive
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from b3216b1 to d07e6ea Compare October 22, 2024 23:44
Copy link
Member

@victorlin victorlin left a comment

Choose a reason for hiding this comment

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

Reviewed all the contents of converted pages. Will continue reviewing styles next week.

static-site/app/not-found.tsx Outdated Show resolved Hide resolved
static-site/app/contact/page.tsx Outdated Show resolved Hide resolved
static-site/app/styles/globals.css Show resolved Hide resolved
static-site/app/styles/globals.css Show resolved Hide resolved
static-site/app/contact/page.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

I didn't look through the styling changes too closely but just compared the updated pages to prod. I like that the /whoami page's layout matches the rest of the site now 👍

I think the move away from styled-components should make it easier to remove UserContext/UserDataWrapper completely and just fetch the current user data server side.

static-site/components/footer/site-map.module.css Outdated Show resolved Hide resolved
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from ffe9b13 to a331d36 Compare November 5, 2024 20:32
@genehack genehack temporarily deployed to nextstrain-s-convert-bl-98fca6 November 5, 2024 20:33 Inactive
static-site/components/focus-paragraph/styles.module.css Outdated Show resolved Hide resolved
static-site/components/nav/nav-logo.tsx Outdated Show resolved Hide resolved
static-site/components/focus-paragraph/styles.module.css Outdated Show resolved Hide resolved
static-site/components/nav/styles.css Outdated Show resolved Hide resolved
@genehack
Copy link
Contributor Author

genehack commented Nov 7, 2024

(Not sure why I can't quote reply this one comment, but...)

My biggest concern is the lack of clarity around where the duplicates are.

I guess I don't see this as a huge issue, because when all this work is done, all the duplicates will be gone, as all the "old" components will be removed/replaced — more specifically, static-site/pages and static-site/src will no longer be present.

If the work is introducing multiple duplicates where there is currently only a single copy, I would consider that a bug/mis-implementation — but I don't think that's the case.

The only way to do this without having some transient duplication would be to convert the entire static-site directory at once, and even then, you'd have to decide between the equally unpalatable options of "have some commits where the site is broken" and "have some commits that are far too large and change multiple things at once due to shared component usage". Compared to those two options, I think having some duplication present in the tree in some commits is the least bad option.

@victorlin
Copy link
Member

(Not sure why I can't quote reply this one comment, but...)

Oh I replied to an existing thread so you have to go back to the thread to add a comment. GitHub UI doesn't do a great job of showing this... copying back to the thread: #1032 (comment)

_Largely_ derived from material in Pages Router pages and components,
but with a number of redundant or otherwise unneeded styling elements
excised for minimalism.

Does not yet implement logged in state, and explicitly drops
"minimized" as a nav-bar concept, at least for the moment.
Note: this includes some updates to the FocusParagraph component which
required some knock-on changes in the 'contact' page, due to the
exports of FocusParagraph changing.
Also updates the 404 content test to match the new behavior of not
returning the full page.

Note: file renaming (`404.jsx` -> `not-found.tsx`) is because Next.js
App Router requires the latter name.

Note: old `pages/404.jsx` and `src/pages/404.jsx` are left in place
because the `ListResources` component depends on the `ErrorContainer`
style being exported from them. This will be addressed in a subsequent
commit.
* Add <UserDataWrapper> component to base layout to provide access to
  logged in user via React Context
* Wrap username or login link display into <UserOrLoginLink> Client
  Component; add to <Nav> component
Note: updates the page to use the default layout with the toolbar and
team avatars, in addition to the sponsor logos.
@genehack genehack force-pushed the convert-blog-to-app-router-134 branch from a331d36 to f8982b0 Compare November 8, 2024 21:34
@genehack genehack temporarily deployed to nextstrain-s-convert-bl-98fca6 November 8, 2024 21:35 Inactive
@genehack genehack merged commit 2c060b4 into master Nov 8, 2024
7 checks passed
@genehack genehack deleted the convert-blog-to-app-router-134 branch November 8, 2024 23:06
@victorlin victorlin mentioned this pull request Nov 13, 2024
2 tasks
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.

6 participants