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

Feat: Blog Posts new 3 column template #157

Closed
wants to merge 16 commits into from

Conversation

flavioriper
Copy link
Contributor

Description

We need this to change the Blog Posts template to 3 columns, including the new Related Posts layout and call to action buttons

Copy link

github-actions bot commented Nov 17, 2023

Visit the preview URL for this PR (updated for commit d49aeb5):

https://estuary-marketing--pr157-feat-blog-post-chang-uaxyxutx.web.app

(expires Fri, 22 Dec 2023 20:09:31 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 76f6b095a0752e5d9c6c890267f9fdc3e392161e

@flavioriper flavioriper requested a review from jshearer November 21, 2023 17:51
@travjenkins travjenkins linked an issue Mar 14, 2024 that may be closed by this pull request
2 tasks
<Link to={`/${article?.slug}`}>{article?.title}</Link>
</li>
</>
<li key={index}>
Copy link
Member

Choose a reason for hiding this comment

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

This should be switched to something like populate-articles__${article.slug}.

Then if the article is actually missing we should just return null as there is no reason to include the element if there is not an article.

<div className="table-of-contents">
<ul>
{items.map((item, index) => (
<li className={`${!item.root ? "sub-item" : ""} ${(index === 0 || item.position <= position) ? "active" : ""}`} key={index}>
Copy link
Member

Choose a reason for hiding this comment

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

key needs expanded

{item.children.map(child => (
<NavMenuList item={child} />
{item.children.map((child, index) => (
<NavMenuList item={child} key={index} />
Copy link
Member

Choose a reason for hiding this comment

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

key needs expanded

<Link
to={`/${post.Slug}`}
className="related-post-card"
key={index}
Copy link
Member

Choose a reason for hiding this comment

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

key needs expanded

Comment on lines +23 to +34
<div className="hero-image">
{post.hero ? (
<GatsbyImage
alt={post.title}
image={
post.hero.localFile.childImageSharp
.gatsbyImageData
}
loading="eager"
/>
) : null}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we are still including the div as empty when there is no hero image?

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

This branch needs updated. I reviewed it in isolation - so a few of the callouts I think would get fixed by is getting updated.

@travjenkins travjenkins added the enhancement New feature or request label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blog Page Template
2 participants