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

✨ Site Nav from tag graph #4439

Merged
merged 16 commits into from
Feb 20, 2025
Merged

✨ Site Nav from tag graph #4439

merged 16 commits into from
Feb 20, 2025

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Jan 11, 2025

Removes the hardcoded SiteNavigationStatic object that powered the site nav and replaces it with a JSON file representation of the tag graph from the database.

As a reminder, the tag graph shape is:

interface TagGraphNode {
    children: TagGraphNode[]
    id: number
    isTopic: boolean
    name: string
    path: number[]
    slug: string | null
    weight: number
}

@owidbot
Copy link
Contributor

owidbot commented Jan 11, 2025

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-sitenav-from-tag-graph

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2025-01-13 22:01:06 UTC
Execution time: 1.25 seconds

@ikesau ikesau force-pushed the sitenav-from-tag-graph branch from e9a303e to 9af48d6 Compare January 13, 2025 21:49
isOnHomepage = props?.content?.type === OwidGdocType.Homepage
}
ReactDOM.render(
<SiteNavigation
Copy link
Member Author

Choose a reason for hiding this comment

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

Technically we don't need to fetch the topicTagGraph.json when we're on the homepage, because it's already embedded in the homepage gdoc's homepageMetadata, but seeing as the JSON request is ~4kB transferred, I think it's okay to not optimize.

Later, we should see if it's possible to get this file and dods.json to cache.

@ikesau ikesau requested a review from mlbrgl January 13, 2025 22:01
@ikesau ikesau marked this pull request as ready for review January 13, 2025 22:01
@ikesau ikesau force-pushed the sitenav-from-tag-graph branch from 9af48d6 to ad4d4b7 Compare January 16, 2025 19:11
Copy link
Member

@mlbrgl mlbrgl left a comment

Choose a reason for hiding this comment

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

nice to see the merging of tag and topics taking shape more visibly!

It seems like the order of tags in the graph is a bit arbitrary at the moment on live, are we waiting for authors to reorder before merging?

@ikesau ikesau force-pushed the sitenav-from-tag-graph branch 2 times, most recently from ce8192c to 445e50f Compare January 24, 2025 23:01
@ikesau ikesau force-pushed the sitenav-from-tag-graph branch from 445e50f to ad88120 Compare January 31, 2025 22:42
@ikesau ikesau requested review from mlbrgl and removed request for mlbrgl January 31, 2025 22:42
@owid owid deleted a comment from socket-security bot Feb 5, 2025
@ikesau ikesau force-pushed the sitenav-from-tag-graph branch from ad88120 to 1f7a2ae Compare February 18, 2025 21:05
@@ -0,0 +1,47 @@
import { useEffect, useRef } from "react"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a rename of SiteMobileCategory - if you omit the commit where that change was made, you can see the actual changes to the file I made

@ikesau ikesau requested a review from mlbrgl February 18, 2025 22:01
@ikesau
Copy link
Member Author

ikesau commented Feb 18, 2025

Hey @mlbrgl - thanks for taking another look at this - the main difference is the flattenNonTopicNodes function and ensuing integration work. I wrote some very basic tests for it to help document what it does. The difference that you'll notice on staging is that the homepage all-topics section now has sub areas again, and topics in the site nav are correctly ordered/grouped.

Copy link
Member

@mlbrgl mlbrgl left a comment

Choose a reason for hiding this comment

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

a couple of very minor comments, everything looks good!

className="homepage-topic__subtopic"
key={`subarea-${name}`}
>
{name}
Copy link
Member

Choose a reason for hiding this comment

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

not sure about the design here but we had a ":" after subarea names.

Incidentally, subareas don't show on staging, but ok on dev (some rebaking/caching shenanigans?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the tag graph on staging doesn't have the latest structure that prod does. It's a pain to re-import, but if I tore down and recreated this staging env it would use the prod data which is correct 👍

@ikesau ikesau force-pushed the sitenav-from-tag-graph branch from e94d617 to 2247f67 Compare February 20, 2025 14:57
@ikesau ikesau merged commit 8c43dae into master Feb 20, 2025
27 checks passed
@ikesau ikesau deleted the sitenav-from-tag-graph branch February 20, 2025 15:54
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.

3 participants