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

Blog page improvements #18

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Blog page improvements #18

wants to merge 16 commits into from

Conversation

tejash-patel
Copy link
Contributor

Various improvements on the blog page:

  • Add default profile picture
  • Update different font sizes - headings and paragraphs
  • start using styled components
  • make individual posts mobile friendly

Copy link
Member

@dellisd dellisd left a 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.

Comment on lines 64 to 85
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()
Copy link
Member

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.

authors,
primary_author_name
}: BlogCardProps) => {
console.log(feature_image)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(feature_image)

This should be removed before merging

@@ -20,7 +40,7 @@ export default ({ data: { ghostPost: post } }) => {
'NOV',
'DEC'
]

console.log(post)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(post)

@tejash-patel
Copy link
Contributor Author

Thanks Derek.

  • Added helper function for common code
  • Update page width to match other pages
  • Add border to posts for neatness

@tejash-patel
Copy link
Contributor Author

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

We can leave this file as css only, can't imagine thinking of different constant names for all these classes!

Copy link
Contributor

@walsquared walsquared left a comment

Choose a reason for hiding this comment

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

It looks like the feature image is being skewed.

The Blog:
image

How it should look:
image

Copy link
Member

@dellisd dellisd left a 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!

Comment on lines 100 to 102
author.profile_image
? author.profile_image
: ProfileIcon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
author.profile_image
? author.profile_image
: ProfileIcon
author.profile_image ?? ProfileIcon

Comment on lines 146 to 148
author.profile_image
? author.profile_image
: ProfileIcon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
author.profile_image
? author.profile_image
: ProfileIcon
author.profile_image ?? ProfileIcon

@@ -0,0 +1,27 @@
export function formatDate(published_at: any) {
Copy link
Member

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?

@@ -0,0 +1,50 @@
import React from 'react'

const WordLogo = (props: any) => (
Copy link
Member

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?

Comment on lines 68 to 70
author.profile_image
? author.profile_image
: ProfileIcon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
author.profile_image
? author.profile_image
: ProfileIcon
author.profile_image ?? ProfileIcon

Comment on lines 70 to 72
posts[0].custom_excerpt
? posts[0].custom_excerpt
: posts[0].excerpt.substring(0, 175)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
posts[0].custom_excerpt
? posts[0].custom_excerpt
: posts[0].excerpt.substring(0, 175)
posts[0].custom_excerpt ?? posts[0].excerpt.substring(0, 175)

@tejash-patel
Copy link
Contributor Author

Updated those changes. Thank you Derek and Wal.

Comment on lines +44 to +52
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>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Comment on lines +85 to +95
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>
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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[]
}

Comment on lines +50 to +52
tags: Array<any>
primary_author_name: String
authors: Array<any>
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants