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

ft/Engineering Blog: Render a blog #868

Merged
merged 24 commits into from
Sep 7, 2024
Merged

ft/Engineering Blog: Render a blog #868

merged 24 commits into from
Sep 7, 2024

Conversation

kelvinkipruto
Copy link
Contributor

@kelvinkipruto kelvinkipruto commented Sep 2, 2024

Description

This PR enables the rendering of blogs in the engineering blog

Fixes #848 #849

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

image
image
image
image
image

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@kelvinkipruto kelvinkipruto marked this pull request as ready for review September 3, 2024 12:01
@kilemensi
Copy link
Member

Will let others look at the code @kelvinkipruto but my vote is on a blog design that uses image sparingly i.e. only when it can improve on words. React, Next.js, etc.

Copy link
Contributor

@koechkevin koechkevin left a comment

Choose a reason for hiding this comment

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

👍

Signed-off-by: Kipruto <[email protected]>
@kelvinkipruto
Copy link
Contributor Author

@koechkevin @DavidTheProgrammer @m453h @thepsalmist @VinneyJ What do you think of @kilemensi comment above, to make it have less images?

Will let others look at the code @kelvinkipruto but my vote is on a blog design that uses image sparingly i.e. only when it can improve on words. React, Next.js, etc.

@DavidTheProgrammer
Copy link
Contributor

@koechkevin @DavidTheProgrammer @m453h @thepsalmist @VinneyJ What do you think of @kilemensi comment above, to make it have less images?

Will let others look at the code @kelvinkipruto but my vote is on a blog design that uses image sparingly i.e. only when it can improve on words. React, Next.js, etc.

I think the image quantity is fine. I like I think it looks really nice 🚀 . Looking through the rest of the codebase at the moment.

Copy link
Contributor

@m453h m453h left a comment

Choose a reason for hiding this comment

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

We might need to add a copy to clipboard button for the syntax highlighter like this one below:

Screenshot 2024-09-03 at 16 57 37

Copy link
Contributor

@VinneyJ VinneyJ left a comment

Choose a reason for hiding this comment

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

LGTM

@VinneyJ VinneyJ self-requested a review September 3, 2024 14:44
Copy link
Contributor

@VinneyJ VinneyJ left a comment

Choose a reason for hiding this comment

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

The detail header image looks too large. What do you think of image width being the same size as the centered content? Otherwise, the code looks good to me.

Copy link
Contributor

@DavidTheProgrammer DavidTheProgrammer left a comment

Choose a reason for hiding this comment

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

Looks good. Just wondering if we decided against using contentlayer and went for the manual approach

apps/engineeringblog/utils/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@DavidTheProgrammer DavidTheProgrammer left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@m453h
Copy link
Contributor

m453h commented Sep 4, 2024

@koechkevin @DavidTheProgrammer @m453h @thepsalmist @VinneyJ What do you think of @kilemensi comment above, to make it have less images?

Will let others look at the code @kelvinkipruto but my vote is on a blog design that uses image sparingly i.e. only when it can improve on words. React, Next.js, etc.

Putting an image with each post is alright, but form my observation as @VinneyJ pointed out, the header image when you open a post is too large and might force us to use high-res images (which increases the size of the page), we could take inspiration from Github Blog though we need to think of the standard of images we will be using for each post. We would really need to emphasize on consistency of the images we select. Something like Vercel Blog

===
Something to take note is that a minimalistic approach of no images saves us the headache of needing to select an appropriate image for each post and means we only get to focus on writing content.

@kilemensi
Copy link
Member

Something to take note is that a minimalistic approach of no images saves us the headache of needing to select an appropriate image for each post and means we only get to focus on writing content.

Finally there!!! Use an image when/if and only when/if it improves the post and it should be part of the content and not decoration.

But, I'll let y'all decide.

@kelvinkipruto
Copy link
Contributor Author

Something to take note is that a minimalistic approach of no images saves us the headache of needing to select an appropriate image for each post and means we only get to focus on writing content.

Let me remove the image from the single blog page

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

👍🏽


This is last sprint's work. Can we get it over the line so folks can start working on this sprint's tasks?

apps/engineeringblog/app/page.tsx Show resolved Hide resolved
apps/engineeringblog/components/Article/ArticleCard.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Empty/Empty.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Article/ArticleCard.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Article/ArticleCard.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Markdown/CodeCopyBtn.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Markdown/CodeCopyBtn.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/components/Markdown/CodeCopyBtn.tsx Outdated Show resolved Hide resolved
apps/engineeringblog/utils/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@m453h m453h left a comment

Choose a reason for hiding this comment

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

This Looks good 🚀

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

I think you've got enough reviews for us to proceed, no?

Signed-off-by: Kipruto <[email protected]>
@kelvinkipruto kelvinkipruto added this pull request to the merge queue Sep 7, 2024
Merged via the queue into main with commit 8876e47 Sep 7, 2024
4 checks passed
@kelvinkipruto kelvinkipruto deleted the ft/render-blog branch September 7, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

@engineeringblog - add layout for blog articles
6 participants