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

Disable About page prefetching #1938

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Disable About page prefetching #1938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c9caa0
Disable about page prefetching
jonkafton Dec 20, 2024
fb910e7
Scroll to top of page on navigate
jonkafton Jan 3, 2025
325df02
Re-enable scroll to fragment
jonkafton Jan 7, 2025
69eda76
Update import rules for Next.js project structure
jonkafton Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions frontends/main/src/app-pages/AboutPage/AboutPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@ import {
theme,
styled,
} from "ol-components"
import * as urls from "@/common/urls"
import { HOME, ABOUT_NON_DEGREE_LEARNING_FRAGMENT } from "@/common/urls"
import React from "react"
import domeImage from "@/public/mit-dome-2.jpg"
import Image from "next/image"

const WHAT_IS_MIT_OPEN_FRAGMENT_IDENTIFIER = "what-is-mit-learn"
const NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER = "non-degree-learning"
const ACADEMIC_AND_PROFESSIONAL_CONTENT = "kinds-of-content"

const SITE_NAME = process.env.NEXT_PUBLIC_SITE_NAME
Expand Down Expand Up @@ -116,7 +115,7 @@ const AboutPage: React.FC = () => {
<BannerContainerInner>
<Breadcrumbs
variant="light"
ancestors={[{ href: urls.HOME, label: "Home" }]}
ancestors={[{ href: HOME, label: "Home" }]}
current="About Us"
/>
<Typography variant="h3" component="h1">
Expand Down Expand Up @@ -206,7 +205,7 @@ const AboutPage: React.FC = () => {
<Typography
variant="h4"
component="h2"
id={NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER}
id={ABOUT_NON_DEGREE_LEARNING_FRAGMENT}
>
What is non-degree learning at MIT?
</Typography>
Expand Down Expand Up @@ -257,9 +256,4 @@ const AboutPage: React.FC = () => {
)
}

export {
AboutPage,
WHAT_IS_MIT_OPEN_FRAGMENT_IDENTIFIER,
NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER,
ACADEMIC_AND_PROFESSIONAL_CONTENT as WHAT_KINDS_OF_CONTENT_FRAGMENT_IDENTIFIER,
}
export { AboutPage }
1 change: 1 addition & 0 deletions frontends/main/src/common/urls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export const SETTINGS = "/dashboard/settings"
export const SEARCH = "/search"

export const ABOUT = "/about"
export const ABOUT_NON_DEGREE_LEARNING_FRAGMENT = "non-degree-learning"

export const ACCESSIBILITY = "https://accessibility.mit.edu/"

Expand Down
6 changes: 4 additions & 2 deletions frontends/main/src/page-components/HeroSearch/HeroSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
SEARCH_NEW,
SEARCH_POPULAR,
SEARCH_UPCOMING,
ABOUT_NON_DEGREE_LEARNING_FRAGMENT,
} from "@/common/urls"
import {
RiAwardLine,
Expand All @@ -26,7 +27,6 @@ import {
RiTimeLine,
RiVerifiedBadgeLine,
} from "@remixicon/react"
import { NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER } from "@/app-pages/AboutPage/AboutPage"
Copy link
Contributor

Choose a reason for hiding this comment

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

btw: This import goes against our hierearchy rules, and should have been forbidden.

Our lint rule enforcing this seems to be broken for two reasons:

  • pages needs to be renamed app-pages
  • "*/tsconfig.json" should be `"**/tsconfig.json"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout - I've updated the rules. This required moving the carousels config into common. This does import a type from @/page-components/ResourceCarousel/ResourceCarousel. Perhaps we should disallow or allow only for types.

import Image from "next/image"
import { SearchField } from "@/page-components/SearchField/SearchField"

Expand Down Expand Up @@ -221,7 +221,9 @@ const HeroSearch: React.FC<{ imageIndex: number }> = ({ imageIndex }) => {
<Typography>
Explore MIT's{" "}
<BoldLink
href={`${ABOUT}#${NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER}`}
href={`${ABOUT}#${ABOUT_NON_DEGREE_LEARNING_FRAGMENT}`}
prefetch={false}
scroll={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also set the "Non-Degree Learning" to scroll top on navigate as noticed that wasn't working correctly.

In what sense was it not working correctly? Currently works as I expect on https://rc.learn.mit.edu/.

For me, scrolls to here:

Screenshot 2025-01-06 at 1 47 58 PM

It doesn't scroll exactly to the header, but I think that's normal.

Request: Leave scroll behavior as-was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looked off to me because the page isn't long enough for the hash fragment to load scrolled to top. I agree though that that doesn't actually constitute not working correctly. I've changed it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh, I didn't think about it enough to explain why it doesn't go all the way to the top.

That makes sense, though.

>
Non-Degree Learning
</BoldLink>
Expand Down
1 change: 1 addition & 0 deletions frontends/ol-components/src/components/Link/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type LinkProps = LinkStyleProps &
*/
shallow?: boolean
scroll?: boolean
prefetch?: boolean
}

const BaseLink = ({
Expand Down
Loading