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): add author avatars #9131

Merged
merged 6 commits into from
Jun 26, 2023
Merged

feat(blog): add author avatars #9131

merged 6 commits into from
Jun 26, 2023

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Jun 22, 2023

Summary

https://mozilla-hub.atlassian.net/browse/MP-467

Companion mdn-studio PR incoming: https://github.com/mdn/mdn-studio/pull/143

Solution

  • move from using one type for both frontmatter and parsed metadata, to one for frontmatter and one for the parsed metadata
  • support using a separate author frontmatter file, while maintaining backwards compatibility
  • added heading spacing recommendations from Anuja

Screenshots

Before

image

image

After

After screenshots look a little different, because the Inter font doesn't load locally for some reason - that's an issue to debug another time.

image

image


How did you test this change?

  • yarn && yarn dev
  • verified all works on :3000 and :5042
  • commented out asset loading added in server/index.ts
  • verified avatars fail to load on :3000 and :5042
  • ran yarn build:blog in separate terminal
  • verified avatars load again on :3000 and :5042 (this time loading from the build out dir)

https://mozilla-hub.atlassian.net/browse/MP-467

move from using one type for both frontmatter and parsed metadata, to
one for frontmatter and one for the parsed metadata

support using a separate author frontmatter file, while maintaining
backwards compatibility
server/index.ts Fixed Show fixed Hide fixed
@LeoMcA LeoMcA requested a review from fiji-flo June 23, 2023 09:34
Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Sorry I ran out of time. First thing Monday. One general point. We could opt for not having a slug but just relying on the author name. This would allow an easy fallback and seamless migration. Or we magically detect the space in the author name and "translate" it into the slug. No requirement just a thought. Otherwise it looks good, but I wanna check it out and really go over it as these file mangling stuff tends to break sometimes.

server/index.ts Show resolved Hide resolved
@LeoMcA
Copy link
Member Author

LeoMcA commented Jun 23, 2023

I'm always bit suspicious of auto slugifying, and lowercasing a "real" name should work cross-system, but will non-ascii characters? Your comment did make me think we don't currently have any behaviour to handle an incorrect slug - the build will just fail. I guess we don't want that, as we can default to "The MDN Team" - so I added a commit to catch the error and output a warning.

This is all backwards compatible with the existing author metadata, though - no need to deploy the associated mdn-studio PR at the same time, just afterwards. I thought now was the time to feel the pain of migrating the metadata, with relatively few posts, as it'll make having things like author pages and expanded metadata in the future easier.

Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Looks great. Only nit or better question is why not add return types (because TS deducts them?, but that's not the point of types I guess).

build/blog.ts Outdated Show resolved Hide resolved
server/index.ts Show resolved Hide resolved
@LeoMcA
Copy link
Member Author

LeoMcA commented Jun 26, 2023

Good point, I went ahead and added return types, it helped me simplify the code a bit.

@LeoMcA LeoMcA merged commit f8d0251 into main Jun 26, 2023
14 checks passed
@LeoMcA LeoMcA deleted the blog-writer-avatar branch June 26, 2023 14:55
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.

2 participants