Link to author pages in byline of articles #3633
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
mlbrgl
changed the title
mlbrgl
force-pushed
the
byline-linking
branch
2 times, most recently
from
89066fc to
64874f2
Compare
|
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 Edited: 2024-05-22 16:47:03 UTC |
| function _getPrefixedPath(prefix: string, gdoc: OwidGdoc): string { | ||
| function _getPrefixedPath( | ||
| prefix: string, | ||
| gdoc: { slug: string; content: { type?: OwidGdocType } } |
There was a problem hiding this comment.
Can you explain this change? It seems fine because undefined is handled, but I'm just curious about where you encountered an undefined content.type 🙂
There was a problem hiding this comment.
sure! This comes from Byline not having access to the full gdoc, where I wanted to use getCanonicalUrl().
content.type is possibly undefined in OwidGdocPostContent (which is why it is covered in the P.union below). It might be a chicken and egg thing, and we might not need the type to be possibly undefined in the first place, but I didn't look more into it.
| @@ -131,24 +120,19 @@ function OwidTopicPageHeader({ | |||
| <p className="topic-page-header__subtitle body-1-regular col-start-2 span-cols-8"> | |||
| {content.subtitle} | |||
| </p> | |||
| <p className="topic-page-header__byline col-start-2 span-cols-8 col-sm-start-2 span-sm-cols-12"> | |||
There was a problem hiding this comment.
Nitpick, but I think I'd have given Byline a className prop, make it render a <p> instead of a fragment, and handle names.length === 0 in the component itself. I can see there's currently one place where it's being rendered inside a <div> but it's probably correct to have that be a <p> anyway.
| // author is unlinked (which probably gets tangled with the "By: " text | ||
| // above). Additional markup, here in the form of a span, works more | ||
| // reliably. | ||
| if (!author.slug) return <span>{author.title}</span> |
There was a problem hiding this comment.
Just wondering if here we still want to link to the team page as a fallback? Not sure if a decision has already been made here but are we even going to be keeping those pages around? 🤔
There was a problem hiding this comment.
yes we might want to do that in case we're using the fallback "Our World in Data team". But for external contributors (which is rare, granted), this wouldn't be desirable in my opinion. I'd go for wait and see on this one.
| {idx === names.length - 1 | ||
| ? "" | ||
| : idx === names.length - 2 | ||
| ? " and " |
There was a problem hiding this comment.
Pretty sure we'd want the oxford comma here
There was a problem hiding this comment.
my thinking behind it was:
- it seems like the main reason for it is to avoid confusion. Here, given that we don't have a real sentence, there isn't any possible confusion
- we currently don't have it in the byline
- it requires special handling when we only have two authors and don't want the oxford comma.
I won't change it for now, but let me know if you see things differently?
mlbrgl
force-pushed
the
remove-progress-bar-throttling
branch
from
8ee226e to
a860437
Compare
fix(byline): hydration issues
see PoC in #3626
Adds links to author pages in byline of articles. Linear and modular topic pages got also upgraded as a side-effect, although they weren't originally part of the scope.
Testing
Only published authors (as in published author pages) are linked to. The rest is output as text.
Validation
Warnings are output in the admin for unlinked authors. This is not a critical error as some authors, e.g. guest authors, won't have a profile on the site.
Post-launch