Add new about page template and people component #4217
Conversation
rakyi
changed the title
rakyi
force-pushed
the
about-pages-base
branch
2 times, most recently
from
a4951ab to
b13313e
Compare
rakyi
force-pushed
the
about-pages-base
branch
3 times, most recently
from
f6868c2 to
1888a1a
Compare
db/model/Gdoc/GdocBase.ts
Outdated
| @@ -255,7 +255,25 @@ export class GdocBase implements OwidGdocBaseInterface { | |||
| .map((author) => author.featuredImage) | |||
| .filter((filename) => !!filename) as string[] | |||
|
|
|||
| return [...this.filenames, ...featuredImages, ...featuredAuthorImages] | |||
| const peopleImages = new Set<string>() | |||
| for (const enrichedBlockSource of this.enrichedBlockSources) { | |||
There was a problem hiding this comment.
I think these images are already getting extracted by extractFilenamesFromBlock 🙂
traverseEnrichedBlock iterates through the persons in a people block already, and extractFilenamesFromBlock gets the image filenames in person blocks 👍
There was a problem hiding this comment.
Good catch! I think I added this before I changed extractFilenamesFromBlock.
| @@ -554,6 +572,8 @@ export class GdocBase implements OwidGdocBaseInterface { | |||
| "list", | |||
| "missing-data", | |||
| "numbered-list", | |||
| "people", | |||
There was a problem hiding this comment.
people has person and person has text, so they should be handled in a with that extracts links from the text 🙂
There was a problem hiding this comment.
I think the comment at the top of this .with() call applies:
// no urls directly on any of these blocks
// their children may contain urls, but they'll be addressed by traverseEnrichedBlock
The links seem to work fine. AFAICT, we'd need a special handling for them here only if they were added as a property on a block, e.g. if {.person} had a url: property.
There was a problem hiding this comment.
Aha whoops! Hoisted by my own petard. Wrote this one before I saw the update to traverseEnrichedBlocks and then didn't think to delete it.
db/model/Gdoc/rawToEnriched.ts
Outdated
|
|
||
| if (typeof raw.value === "string") | ||
| return createError({ | ||
| message: "Value is a string, not an array of people", |
There was a problem hiding this comment.
This check is always kind of contrived and unlikely to trip, but given that these errors are mostly for the authors, including the correct syntax in the error message will possibly save everyone some time.
| @@ -0,0 +1,6 @@ | |||
| .people { | |||
There was a problem hiding this comment.
Did you consider implementing this layout with the CSS grid helpers and find it overly complicated?
It seems fine here (and is admittedly much simpler, plus avoids the margin collapsing issue) but I worry about having multiple arbitrary ways of doing layout stuff. One nice thing about sticking to the column system is that everything will line up to a single grid (which is the way that Marwa works) without the need for magic numbers.
Interested in your thoughts on this 🙂
There was a problem hiding this comment.
Good question, I should have mentioned it in the PR description. I did consider it, but decided against it because we'll have three different layouts for people configurable in archie: 1, 2 and 4 columns and on small screens they need to collapse into a single column row by row, e.g. first all the people from the first row, then all people from the second row etc.
I'll extend the component in a future PR.
There was a problem hiding this comment.
Okay! Can we take a look at it together when you get around to implementing the stacking layout? I just wanna make sure I understand if there's a way in which the grid system isn't working for us 👍
There was a problem hiding this comment.
On a second thought, we might be able to use the grid system at least partially. You'll either see it in the next PR or I'll talk to you before, if it's gonna be a significant amount of work.
rakyi
force-pushed
the
about-pages-base
branch
from
0e92ed3 to
b682a3d
Compare
rakyi
force-pushed
the
about-pages-base
branch
from
b682a3d to
874da67
Compare
Other components and content will follow in future PRs.