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

✨ Site Nav from tag graph #4439

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

✨ Site Nav from tag graph #4439

wants to merge 8 commits into from

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Jan 11, 2025 •
edited
Loading

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 •
edited
Loading

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
ikesau
ikesau commented Jan 13, 2025
View reviewed changes
site/runSiteNavigation.tsx
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
mlbrgl
mlbrgl approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@mlbrgl mlbrgl left a comment •
edited
Loading

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?

site/gdocs/pages/Homepage.tsx
Comment on lines +86 to +93
{area.children.map(({ slug, name }) => (
<li
className="homepage-topic__topic-entry"
key={`topic-entry-${slug}`}
>
<a href={`/${slug}`}>{name}</a>
</li>
))}
Copy link
Member

Choose a reason for hiding this comment

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

we dropped subcategories then? I'm guessing yes since they're not part of the tag graph, but just wanted to check.

db/db.ts
SELECT COUNT(DISTINCT(t.slug))
FROM tags t
LEFT JOIN posts_gdocs p ON t.slug = p.slug
WHERE t.slug IS NOT NULL AND p.published IS TRUE`
Copy link
Member

Choose a reason for hiding this comment

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

for coherence in the defintion of topic, add the p.type IN ('topic-page', 'linear-topic-page', 'article') condition from getFlatTagGraph()?

packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts
Copy link
Member

Choose a reason for hiding this comment

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

there is a couple of edits here related to narrative charts, seems incidental but checking

db/model/Post.ts
)
)
})
return slug === "coronavirus"
Copy link
Member

Choose a reason for hiding this comment

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

I ran a local bake to check this, and these are the slugs calling isPostSlugCitable:

  • history-of-poverty-data-appendix
  • food-explorers
  • privacy-policy
  • thank-you
  • donations-faq
  • glossary
  • about
  • former-team-members
  • about
  • owid-grapher
  • about
  • coverage
  • teaching
  • ukraine-war

None of these are citable, so that function can just be a noop until we do a bigger refactor.

"coronavirus" (as in the landing for country profiles, e.g. /coronavirus/country/usa) is not listed here because country profiles landing pages are assumed to be citable, and don't run through that function.

site/SiteNavigationTopics.tsx
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe worth renaming "category" -> "area" here too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlbrgl mlbrgl mlbrgl approved these changes

Assignees

@ikesau ikesau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ikesau @owidbot @mlbrgl