-
Notifications
You must be signed in to change notification settings - Fork 1
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
Blog page improvements #18
base: master
Are you sure you want to change the base?
Conversation
also rename the file to use camel case
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.
Overall the code looks good other than a bit of duplicate code that can be pulled out.
A couple of general points though:
- The width of the blog should be increased to match the Events page. Right now the blog is much narrower.
- There isn't enough distinction with the blog cards (this is an issue with the original design, not the code). It can be hard to tell where one card starts and one card ends. The "featured" card also looks like it's floating in the middle of nowhere (for lack of a better term).
- Is
postLayout.css
necessary? I feel like in a lot of cases it would make more sense to extract as many of those css classes as possible and convert them to styled components. I understand it's a lot of css though.
src/components/blogCard.tsx
Outdated
const months = [ | ||
'JAN', | ||
'FEB', | ||
'MAR', | ||
'APR', | ||
'MAY', | ||
'JUN', | ||
'JULY', | ||
'AUG', | ||
'SEPT', | ||
'OCT', | ||
'NOV', | ||
'DEC' | ||
] | ||
|
||
const month = new Date(published_at).getMonth() | ||
const formatted_published_at = | ||
new Date(published_at).getDate() + | ||
' ' + | ||
months[month] + | ||
' ' + | ||
new Date(published_at).getFullYear() |
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.
This could be pulled out as a helper function since it's used in multiple places.
src/components/blogCard.tsx
Outdated
authors, | ||
primary_author_name | ||
}: BlogCardProps) => { | ||
console.log(feature_image) |
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.
console.log(feature_image) |
This should be removed before merging
src/layouts/postLayout.tsx
Outdated
@@ -20,7 +40,7 @@ export default ({ data: { ghostPost: post } }) => { | |||
'NOV', | |||
'DEC' | |||
] | |||
|
|||
console.log(post) |
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.
console.log(post) |
Thanks Derek.
|
We can leave this file as css only, can't imagine thinking of different constant names for all these classes! |
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.
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 ??
operator is now your best friend!
src/components/blogCard.tsx
Outdated
author.profile_image | ||
? author.profile_image | ||
: ProfileIcon |
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.
author.profile_image | |
? author.profile_image | |
: ProfileIcon | |
author.profile_image ?? ProfileIcon |
src/components/blogCardFeatured.tsx
Outdated
author.profile_image | ||
? author.profile_image | ||
: ProfileIcon |
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.
author.profile_image | |
? author.profile_image | |
: ProfileIcon | |
author.profile_image ?? ProfileIcon |
src/components/helpers.tsx
Outdated
@@ -0,0 +1,27 @@ | |||
export function formatDate(published_at: any) { |
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.
Could we specify a proper type for this parameter instead of any
?
src/components/wordLogo.tsx
Outdated
@@ -0,0 +1,50 @@ | |||
import React from 'react' | |||
|
|||
const WordLogo = (props: any) => ( |
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.
Can you specify a proper interface
for these props and use that instead of any
?
src/layouts/postLayout.tsx
Outdated
author.profile_image | ||
? author.profile_image | ||
: ProfileIcon |
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.
author.profile_image | |
? author.profile_image | |
: ProfileIcon | |
author.profile_image ?? ProfileIcon |
src/pages/blog.tsx
Outdated
posts[0].custom_excerpt | ||
? posts[0].custom_excerpt | ||
: posts[0].excerpt.substring(0, 175) |
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.
posts[0].custom_excerpt | |
? posts[0].custom_excerpt | |
: posts[0].excerpt.substring(0, 175) | |
posts[0].custom_excerpt ?? posts[0].excerpt.substring(0, 175) |
Updated those changes. Thank you Derek and Wal. |
title: String | ||
reading_time: number | ||
published_at: Date | ||
excerpt: String | ||
feature_image: String | ||
slug: String | ||
tags: Array<any> | ||
primary_author_name: String | ||
authors: Array<any> |
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.
title: String | |
reading_time: number | |
published_at: Date | |
excerpt: String | |
feature_image: String | |
slug: String | |
tags: Array<any> | |
primary_author_name: String | |
authors: Array<any> | |
title: string | |
reading_time: number | |
published_at: Date | |
excerpt: string | |
feature_image: string | |
slug: string | |
tags: any[] | |
primary_author_name: string | |
authors: any[] |
Use primitive types
type BlogCardProps = { | ||
title: String | ||
reading_time: number | ||
published_at: Date | ||
excerpt: String | ||
feature_image: String | ||
slug: String | ||
tags: Array<any> | ||
primary_author_name: String | ||
authors: Array<any> | ||
} |
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.
type BlogCardProps = { | |
title: String | |
reading_time: number | |
published_at: Date | |
excerpt: String | |
feature_image: String | |
slug: String | |
tags: Array<any> | |
primary_author_name: String | |
authors: Array<any> | |
} | |
type BlogCardProps = { | |
title: string | |
reading_time: number | |
published_at: Date | |
excerpt: string | |
feature_image: string | |
slug: string | |
tags: any[] | |
primary_author_name: string | |
authors:any[] | |
} |
tags: Array<any> | ||
primary_author_name: String | ||
authors: Array<any> |
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.
Can you also specify interfaces for Tags and Authors? (and any other interfaces that might be required).
Various improvements on the blog page: