🎉 Homepage redesign #3219
🎉 Homepage redesign #3219
Conversation
🎉 Homepage Gdoc type
🎉 Homepage Search archie component
🎉 Homepage Intro archie component
🎉 Homepage static content
WalkthroughThe updates encompass a holistic approach to support homepage content management and display throughout the project. These changes include expanding file size limits, introducing new content types like the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| .article-block__pill-row { | ||
| background-color: $blue-20; | ||
| scrollbar-width: thin; | ||
| overflow-x: auto; | ||
| .article-block__pill-row-interior { | ||
| display: flex; | ||
| padding: 8px 0; | ||
| } | ||
| p { | ||
| color: $blue-60; | ||
| margin: 0 9px 0 0; | ||
| line-height: 32px; | ||
| white-space: nowrap; | ||
| } | ||
| ul { | ||
| list-style: none; | ||
| display: flex; | ||
| flex-wrap: nowrap; | ||
| } | ||
| .article-block__pill { | ||
| &:not(:last-child) { | ||
| margin-right: 8px; | ||
| } | ||
| a { | ||
| color: $blue-100; | ||
| background-color: #fff; | ||
| line-height: 2rem; | ||
| padding: 0 16px; | ||
| display: inline-block; | ||
| transition: background-color 250ms; | ||
| border-radius: 32px; | ||
| white-space: nowrap; | ||
| &:hover { | ||
| background-color: $gray-10; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The styling for .article-block__pill-row is well-structured and follows SCSS best practices. However, consider using variables for repeated values such as margin sizes and colors to enhance maintainability and consistency across the stylesheet.
- margin: 0 9px 0 0;
+ margin: 0 $pill-row-margin-right 0 0; // Assuming $pill-row-margin-right: 9px; is defined at the top
- background-color: #fff;
+ background-color: $white; // Assuming $white: #fff; is defined at the top
- background-color: $gray-10;
+ background-color: $pill-hover-bg-color; // Assuming $pill-hover-bg-color: $gray-10; is defined at the topCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| .article-block__pill-row { | |
| background-color: $blue-20; | |
| scrollbar-width: thin; | |
| overflow-x: auto; | |
| .article-block__pill-row-interior { | |
| display: flex; | |
| padding: 8px 0; | |
| } | |
| p { | |
| color: $blue-60; | |
| margin: 0 9px 0 0; | |
| line-height: 32px; | |
| white-space: nowrap; | |
| } | |
| ul { | |
| list-style: none; | |
| display: flex; | |
| flex-wrap: nowrap; | |
| } | |
| .article-block__pill { | |
| &:not(:last-child) { | |
| margin-right: 8px; | |
| } | |
| a { | |
| color: $blue-100; | |
| background-color: #fff; | |
| line-height: 2rem; | |
| padding: 0 16px; | |
| display: inline-block; | |
| transition: background-color 250ms; | |
| border-radius: 32px; | |
| white-space: nowrap; | |
| &:hover { | |
| background-color: $gray-10; | |
| } | |
| } | |
| } | |
| } | |
| .article-block__pill-row { | |
| background-color: $blue-20; | |
| scrollbar-width: thin; | |
| overflow-x: auto; | |
| .article-block__pill-row-interior { | |
| display: flex; | |
| padding: 8px 0; | |
| } | |
| p { | |
| color: $blue-60; | |
| margin: 0 $pill-row-margin-right 0 0; // Assuming $pill-row-margin-right: 9px; is defined at the top | |
| line-height: 32px; | |
| white-space: nowrap; | |
| } | |
| ul { | |
| list-style: none; | |
| display: flex; | |
| flex-wrap: nowrap; | |
| } | |
| .article-block__pill { | |
| &:not(:last-child) { | |
| margin-right: 8px; | |
| } | |
| a { | |
| color: $blue-100; | |
| background-color: $white; // Assuming $white: #fff; is defined at the top | |
| line-height: 2rem; | |
| padding: 0 16px; | |
| display: inline-block; | |
| transition: background-color 250ms; | |
| border-radius: 32px; | |
| white-space: nowrap; | |
| &:hover { | |
| background-color: $pill-hover-bg-color; // Assuming $pill-hover-bg-color: $gray-10; is defined at the top | |
| } | |
| } | |
| } | |
| } |
baker/siteRenderers.tsx
Outdated
| // This function is temporarily forked until we have fully transitioned to a gdocs homepage, | ||
| // whereupon we'll strip out the old front page code. | ||
| export const renderFrontPage = async () => { | ||
| // Annoying, MySQL+TypeORM doesn't support JSONB, so I'm using raw SQL to confirm if there's a published homepage | ||
| const gdocHomepageResult = await db.knexRawFirst<{ id: string }>( | ||
| `SELECT id FROM posts_gdocs WHERE content->>"$.type" = "${OwidGdocType.Homepage}" AND published = TRUE`, | ||
| db.knexInstance() | ||
| ) | ||
|
|
||
| if (gdocHomepageResult) { | ||
| const gdocHomepage = await GdocFactory.load(gdocHomepageResult.id) | ||
| if (gdocHomepage) { | ||
| await gdocHomepage.loadState() | ||
| return renderGdoc(gdocHomepage) | ||
| } | ||
| } |
There was a problem hiding this comment.
The modifications to the renderFrontPage function introduce a new approach to rendering the homepage by checking for a published homepage Google Doc before proceeding with the existing logic. This change is significant as it affects the initial content users will see. Consider the following points:
- SQL Injection Protection: The SQL query within
db.knexRawFirstuses parameterized inputs, which is a good practice for preventing SQL injection vulnerabilities. However, since this is a critical security aspect, double-check that all dynamic inputs to SQL queries throughout the application follow this practice. - Error Handling: Ensure that there is adequate error handling around the database query and subsequent logic. If the database query fails or if there are issues loading the Google Doc, the application should handle these gracefully, possibly falling back to the default rendering logic.
- Performance Considerations: Depending on the frequency of homepage visits, this database query could potentially impact performance. Consider caching the result of the homepage check or the rendered homepage content itself to reduce database load.
- Maintainability: The comment mentions that this function is temporarily forked and will undergo further changes. Ensure that there is a clear plan and timeline for the transition to the new homepage rendering logic to avoid having temporary fixes become permanent technical debt.
Overall, the approach seems sound, but careful attention should be paid to security, error handling, and performance implications.
Consider implementing caching for the homepage check or rendered content to improve performance. Additionally, ensure robust error handling around the database query and Google Doc loading process.
db/model/Gdoc/rawToEnriched.ts
Outdated
| function parsePillRow(raw: RawBlockPillRow): EnrichedBlockPillRow { | ||
| function createError(error: ParseError): EnrichedBlockPillRow { | ||
| return { | ||
| type: "pill-row", | ||
| parseErrors: [error], | ||
| pills: [], | ||
| title: "", | ||
| } | ||
| } | ||
|
|
||
| if (!raw.value.title) { | ||
| return createError({ | ||
| message: "Pill row is missing a title", | ||
| }) | ||
| } | ||
| if (!raw.value.pills) { | ||
| return createError({ | ||
| message: "Pill row is missing pills", | ||
| }) | ||
| } | ||
|
|
||
| const pills: { text: string; url: string }[] = [] | ||
|
|
||
| for (const rawPill of raw.value.pills) { | ||
| const url = extractUrl(rawPill.url) | ||
| if (!url) { | ||
| return createError({ | ||
| message: "A pill is missing a url", | ||
| }) | ||
| } | ||
|
|
||
| if (!rawPill.text && getLinkType(url) !== "gdoc") { | ||
| return createError({ | ||
| message: | ||
| "A pill that is linking to a non-gdoc resource is missing text", | ||
| }) | ||
| } | ||
|
|
||
| pills.push({ text: rawPill.text!, url }) | ||
| } | ||
|
|
||
| return { | ||
| type: "pill-row", | ||
| parseErrors: [], | ||
| pills: pills, | ||
| title: raw.value.title, | ||
| } | ||
| } |
There was a problem hiding this comment.
The parsePillRow function correctly parses the pill-row block type from raw to enriched format. However, there's a potential improvement in error handling and validation:
- Consider validating the
urlformat to ensure it's a valid URL. - It might be beneficial to validate the presence of
textfor each pill, even if it links to agdoc, to ensure consistency and clarity in the UI.
for (const rawPill of raw.value.pills) {
const url = extractUrl(rawPill.url)
if (!url) {
return createError({
message: "A pill is missing a url",
})
}
+ // Validate URL format
+ if (!isValidUrl(url)) {
+ return createError({
+ message: `The URL "${url}" is not valid.`,
+ })
+ }
if (!rawPill.text && getLinkType(url) !== "gdoc") {
return createError({
message:
"A pill that is linking to a non-gdoc resource is missing text",
})
}
pills.push({ text: rawPill.text!, url })
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| function parsePillRow(raw: RawBlockPillRow): EnrichedBlockPillRow { | |
| function createError(error: ParseError): EnrichedBlockPillRow { | |
| return { | |
| type: "pill-row", | |
| parseErrors: [error], | |
| pills: [], | |
| title: "", | |
| } | |
| } | |
| if (!raw.value.title) { | |
| return createError({ | |
| message: "Pill row is missing a title", | |
| }) | |
| } | |
| if (!raw.value.pills) { | |
| return createError({ | |
| message: "Pill row is missing pills", | |
| }) | |
| } | |
| const pills: { text: string; url: string }[] = [] | |
| for (const rawPill of raw.value.pills) { | |
| const url = extractUrl(rawPill.url) | |
| if (!url) { | |
| return createError({ | |
| message: "A pill is missing a url", | |
| }) | |
| } | |
| if (!rawPill.text && getLinkType(url) !== "gdoc") { | |
| return createError({ | |
| message: | |
| "A pill that is linking to a non-gdoc resource is missing text", | |
| }) | |
| } | |
| pills.push({ text: rawPill.text!, url }) | |
| } | |
| return { | |
| type: "pill-row", | |
| parseErrors: [], | |
| pills: pills, | |
| title: raw.value.title, | |
| } | |
| } | |
| function parsePillRow(raw: RawBlockPillRow): EnrichedBlockPillRow { | |
| function createError(error: ParseError): EnrichedBlockPillRow { | |
| return { | |
| type: "pill-row", | |
| parseErrors: [error], | |
| pills: [], | |
| title: "", | |
| } | |
| } | |
| if (!raw.value.title) { | |
| return createError({ | |
| message: "Pill row is missing a title", | |
| }) | |
| } | |
| if (!raw.value.pills) { | |
| return createError({ | |
| message: "Pill row is missing pills", | |
| }) | |
| } | |
| const pills: { text: string; url: string }[] = [] | |
| for (const rawPill of raw.value.pills) { | |
| const url = extractUrl(rawPill.url) | |
| if (!url) { | |
| return createError({ | |
| message: "A pill is missing a url", | |
| }) | |
| } | |
| // Validate URL format | |
| if (!isValidUrl(url)) { | |
| return createError({ | |
| message: `The URL "${url}" is not valid.`, | |
| }) | |
| } | |
| if (!rawPill.text && getLinkType(url) !== "gdoc") { | |
| return createError({ | |
| message: | |
| "A pill that is linking to a non-gdoc resource is missing text", | |
| }) | |
| } | |
| pills.push({ text: rawPill.text!, url }) | |
| } | |
| return { | |
| type: "pill-row", | |
| parseErrors: [], | |
| pills: pills, | |
| title: raw.value.title, | |
| } | |
| } |
| export type RawBlockPillRow = { | ||
| type: "pill-row" | ||
| value: { | ||
| title?: string | ||
| pills?: { | ||
| text?: string | ||
| url?: string | ||
| }[] | ||
| } | ||
| } |
There was a problem hiding this comment.
The RawBlockPillRow type definition correctly specifies optional properties for title and pills, which aligns with the flexible nature of content blocks. However, consider adding a comment to clarify the use case for when text might be optional within pills.
| .with({ type: "homepage-intro" }, (block) => { | ||
| return ( | ||
| <HomepageIntro | ||
| className={getLayout("homepage-intro")} | ||
| {...block} | ||
| /> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
The rendering logic for the HomepageIntro component is well-implemented, with the component's props being correctly spread and the className being determined using the getLayout function. However, it seems that the containerType prop is not passed to getLayout for this component, which could lead to inconsistent styling if different container types are intended to affect its layout. Consider passing containerType to ensure consistent styling across different usage contexts.
- <HomepageIntro className={getLayout("homepage-intro")} {...block} />
+ <HomepageIntro className={getLayout("homepage-intro", containerType)} {...block} />Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| .with({ type: "homepage-intro" }, (block) => { | |
| return ( | |
| <HomepageIntro | |
| className={getLayout("homepage-intro")} | |
| {...block} | |
| /> | |
| ) | |
| }) | |
| .with({ type: "homepage-intro" }, (block) => { | |
| return ( | |
| <HomepageIntro | |
| className={getLayout("homepage-intro", containerType)} | |
| {...block} | |
| /> | |
| ) | |
| }) |
| export const getTotalNumberOfTopics = (): Promise<number> => { | ||
| return knexRawFirst<{ count: number }>( | ||
| ` | ||
| SELECT COUNT(DISTINCT(tagId)) AS count | ||
| FROM chart_tags | ||
| WHERE chartId IN ( | ||
| SELECT id | ||
| FROM charts | ||
| WHERE publishedAt IS NOT NULL)`, | ||
| knexInstance() | ||
| ).then((res) => res?.count ?? 0) |
There was a problem hiding this comment.
The getTotalNumberOfTopics function correctly counts the number of unique topics by counting distinct tagId values from chart_tags where the chart is published. It follows best practices for query parameterization. Similar to other functions, adding error handling would enhance its reliability.
export const getTotalNumberOfTopics = (): Promise<number> => {
return knexRawFirst<{ count: number }>(
`SELECT COUNT(DISTINCT(tagId)) AS count
FROM chart_tags
WHERE chartId IN (
SELECT id
FROM charts
WHERE publishedAt IS NOT NULL)`,
knexInstance()
).then((res) => res?.count ?? 0)
+ .catch((error) => {
+ console.error("Failed to get total number of topics:", error);
+ throw error; // Rethrow or handle as appropriate for your application
+ });
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export const getTotalNumberOfTopics = (): Promise<number> => { | |
| return knexRawFirst<{ count: number }>( | |
| ` | |
| SELECT COUNT(DISTINCT(tagId)) AS count | |
| FROM chart_tags | |
| WHERE chartId IN ( | |
| SELECT id | |
| FROM charts | |
| WHERE publishedAt IS NOT NULL)`, | |
| knexInstance() | |
| ).then((res) => res?.count ?? 0) | |
| export const getTotalNumberOfTopics = (): Promise<number> => { | |
| return knexRawFirst<{ count: number }>( | |
| `SELECT COUNT(DISTINCT(tagId)) AS count | |
| FROM chart_tags | |
| WHERE chartId IN ( | |
| SELECT id | |
| FROM charts | |
| WHERE publishedAt IS NOT NULL)`, | |
| knexInstance() | |
| ).then((res) => res?.count ?? 0) | |
| .catch((error) => { | |
| console.error("Failed to get total number of topics:", error); | |
| throw error; // Rethrow or handle as appropriate for your application | |
| }); | |
| } |
| function Pill(props: { text?: string; url: string }) { | ||
| const { linkedDocument, errorMessage } = useLinkedDocument(props.url) | ||
| const { isPreviewing } = useContext(DocumentContext) | ||
| const url = linkedDocument ? `/${linkedDocument.slug}` : props.url | ||
| const text = props.text ?? linkedDocument?.title | ||
|
|
||
| if (isPreviewing) { | ||
| if (errorMessage || !text || !url) { | ||
| return ( | ||
| <li className="article-block__pill article-block__pill--error"> | ||
| {errorMessage} | ||
| </li> | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if (!text || !url) return null | ||
|
|
||
| return ( | ||
| <li className="article-block__pill"> | ||
| <a className="body-3-medium" href={url}> | ||
| {text} | ||
| </a> | ||
| </li> | ||
| ) |
There was a problem hiding this comment.
The Pill component and PillRow function are well-implemented with clear logic for handling linked documents and previewing states. However, consider adding error handling for the useLinkedDocument hook to gracefully handle any potential failures in fetching linked documents.
Consider implementing error handling within the useLinkedDocument hook to improve resilience and user experience.
| _validateSubclass = async (): Promise<OwidGdocErrorMessage[]> => { | ||
| const errors: OwidGdocErrorMessage[] = [] | ||
| const otherPublishedHomepages = await db.knexRaw<{ id: string }>( | ||
| ` | ||
| SELECT | ||
| id | ||
| FROM posts_gdocs | ||
| WHERE content->>"$.type" = "${OwidGdocType.Homepage}" | ||
| AND published = TRUE | ||
| AND id != ?`, | ||
| db.knexInstance(), | ||
| [this.id] | ||
| ) | ||
| if (otherPublishedHomepages.length > 0) { | ||
| errors.push({ | ||
| property: "published", | ||
| message: `There can only be one published homepage. There is a homepage with the ID ${otherPublishedHomepages[0].id} that is already published.`, | ||
| type: OwidGdocErrorMessageType.Error, | ||
| }) | ||
| } | ||
| return errors |
There was a problem hiding this comment.
The validation logic to ensure only one homepage is published at a time is crucial for maintaining site integrity. However, consider optimizing the SQL query to improve performance, especially if the posts_gdocs table grows significantly.
Optimize the SQL query in _validateSubclass for better performance with large datasets.
site/gdocs/pages/Homepage.tsx
Outdated
| @@ -0,0 +1,200 @@ | |||
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome" | |||
There was a problem hiding this comment.
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome" | |
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" |
This saves 10KB on the bundle when replacing all occurrences of "import { FontAwesomeIcon } from @fortawesome/react-fontawesome across the codebase.
site/css/grid.scss
Outdated
| @@ -61,7 +61,7 @@ $grid-responsive: (lg, md, sm); | |||
| // Use this when you want a full-page-width grid container | |||
| // 1 (auto) | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 (auto) | |||
| .grid-cols-12-full-width { | |||
| --outer-column-width: calc((50vw - 640px - var(--grid-gap))); | |||
| --outer-column-width: calc((50vw - 647.5px - var(--grid-gap))); | |||
There was a problem hiding this comment.
could you explain me this change in a few words?
There was a problem hiding this comment.
I actually can't 😭
I have no idea why it's happening but it seems like every couple of months this rule stops working and the 12-column grid is no longer 1280px wide.
load the homepage and look at .latest-data-insights-block__card-container on a desktop - it's 1280px
then change --outer-column-width to what is was before and the element's 1265px wide
but last time I did this I swear it was too large so I bumped it back down. 😭😭
There was a problem hiding this comment.
interesting I'm seeing different widths on Chrome:
- 647.5 -> 1295px
- 640 -> 1280px
| ORDER BY publishedAt DESC | ||
| LIMIT ? | ||
| `, | ||
| knexInstance(), |
There was a problem hiding this comment.
with the recent refactor in #3194, you're now able to get the knex instance from the baker directly
There was a problem hiding this comment.
Just making sure I understand you - you mean all these db functions should take knex as an argument and we pass the same instance around everywhere? 🙂
| SELECT | ||
| content->>'$.title' AS title, | ||
| publishedAt, | ||
| ROW_NUMBER() OVER (ORDER BY publishedAt DESC) - 1 AS \`index\` |
There was a problem hiding this comment.
seems a bit heavy handed, given that we can get the index in the map() below (EDIT: I'm seeing now this has been merely moved so not really part of this PR)
Figma
Test document
There's a big change in bundle size because the new
homepage-searchcomponent moves the algolia library intocommon.mjsThe
FrontPagecode is all still in here (with a few modifications where I overrode/reused/shared CSS) but will be removed once this branch is merged and we've published the new homepage.New Features
homepagegdoc typehomepageexists, it will be baked asindex.html, otherwise we bake the default frontpage.homepageMetadataattachments context (to provide stats on how many charts and topics we have)New components
pill-rowhomepage-searchhomepage-introlatest-data-insightsTODO
Summary by CodeRabbit
Homepagedocument type across the platform, including icons, settings, and preview functionalities.PillRow,HomepageIntro,HomepageSearch, andLatestDataInsightsBlock.SearchPage.dbmodule.