-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
base: master
Are you sure you want to change the base?
Conversation
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 2 (74e005) ❌ Edited: 2024-12-20 22:36:44 UTC |
0342ee6
to
14aac77
Compare
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. |
There was a problem hiding this 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.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", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice tests!
Resolves #3406
Changes:
ParentTagArray
which is an array of tags. One child tag can have multipleParentTagArray
s e.g. "Nuclear Energy" can be a child of "Energy" and in a different part of the graph, a child of "CO2 Emissions"Rules for the programmatic breadcrumbs:
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:
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.