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

🎉 automatic breadcrumbs from the tag graph #4343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Dec 19, 2024

Resolves #3406

Changes:

  • Adds programmatically generated breadcrumbs to our tagged gdocs articles, based on the tag graph
  • Fixes erroneous parent tag logic to reflect the fact that a tag may have multiple paths to get to it
    • This requires a somewhat verbose new structure called a ParentTagArray which is an array of tags. One child tag can have multiple ParentTagArrays e.g. "Nuclear Energy" can be a child of "Energy" and in a different part of the graph, a child of "CO2 Emissions"
    • We want all of these parent tags for Algolia indexing, so the code has been fixed to reflect this

Rules for the programmatic breadcrumbs:

  1. For each tag, pick the root-to-leaf subgraph with the highest weighted top node (tiebreak alphabetically)
  2. For each subgraph, filter out non-topic nodes
  3. Pick the longest filtered subgraph

I considered a "highest average weight" strategy as well, but seeing as weighting is only really used to order nodes relative to one another at each level, I didn't think it would lead to intuitive results.

This means that the ordering of the areas in the tag graph will determine which breadcrumbs trump which. I think this makes the most sense because presumably whichever topic we put at the top of the nav, we think is the most important and therefore its children are more important also, in some abstract way.

Automatic breadcrumbs render slightly differently to manually set breadcrumbs:

image
image

Manually set breadcrumbs were made for SDGs at first, where we wanted the leaf node to unclickable. For non-SDG articles, authors have (sensibly) shortened the title for this leaf node, but that's not something we're going to be able to do programmatically, so instead we'll only show topic breadcrumbs, and not a leaf node for the article.

@ikesau ikesau self-assigned this Dec 19, 2024
@owidbot
Copy link
Contributor

owidbot commented Dec 20, 2024

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-breadcrumbs

SVG tester:

Number of differences (default views): 2 (74e005) ❌
Number of differences (all views): 8 (4588eb) ❌

Edited: 2024-12-20 22:36:44 UTC
Execution time: 1.23 seconds

@ikesau ikesau force-pushed the breadcrumbs branch 2 times, most recently from 0342ee6 to 14aac77 Compare December 20, 2024 22:02
@ikesau ikesau requested a review from danyx23 December 20, 2024 22:02
@ikesau ikesau marked this pull request as ready for review December 20, 2024 22:02
@danyx23
Copy link
Contributor

danyx23 commented Dec 23, 2024

Nice work! A few notes from playing with it: I think it would be great if there were an easier way in the admin to understand if breadcrumbs were set automatically or manually.

The final leaf node not being shown is understandable but quite an inconsistency. It might work well to use the slug, split by dash and sentence cased? E.g. "us-airline-travel" -> "Us airline travel". This will clearly sometimes not be perfect but this is what manual overrides are for. What do you think?

My hunch is also that we should advertise this staging server quite prominently after the christmas break - I'm pretty sure that Joe, Ed and maybe Max will want to look at quite a few examples to get a feeling for the logic. I think it works very well (e.g. the tie breaking etc) but I anticipate some discussion there.

Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

The types in getParentTagArraysByChildName should be a bit more explicit IMHO, otherwise this looks good!

As I wrote above, I think the semantics of this PR will warrant a bit of scrutiy and discussion and this might mean a few more code changes - if so, ping me again quickly before this gets merged.

const tagsById = await trx("tags")
.select("id", "name")
.select("id", "name", "slug")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.select("id", "name", "slug")
const tagsById = await trx<DbPlainTag>("tags")

This should have

@@ -920,3 +933,270 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
expect(json.success).toBe(false)
})
})

describe.only("OwidAdminApp: tag graph", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice tests!

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.

Automate more breadcrumbs
3 participants