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): authors page #10216

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft

feat(blog): authors page #10216

wants to merge 34 commits into from

Conversation

OzakIOne
Copy link
Collaborator

@OzakIOne OzakIOne commented Jun 13, 2024

Pre-flight checklist

  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

#10196

Todo

  • questions above ^
  • questions in the RFC
  • fix todos 🙃
  • write and fix tests
  • polish authors page UI ?
  • fix swizzle ci

Test Plan

Dogfood, unit tests, E2E
- enable argos at the end

Test links

Authors: https://deploy-preview-10216--docusaurus-2.netlify.app/blog/authors
Slorber author: https://deploy-preview-10216--docusaurus-2.netlify.app/blog/authors/slorber

Related issues/PRs

Fix #10196

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 13, 2024
Copy link

github-actions bot commented Jun 13, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 88 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 64 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 69 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 71 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/tags 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report

Copy link

netlify bot commented Jun 13, 2024

[V2]

Name Link
🔨 Latest commit 6b62c98
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/667b275b99887800087082af
😎 Deploy Preview https://deploy-preview-10216--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Jun 13, 2024

Size Change: +14 kB (+0.76%)

Total Size: 1.86 MB

Filename Size Change
website/.docusaurus/docusaurus.config.mjs 27.2 kB +96 B (+0.35%)
website/.docusaurus/registry.js 308 kB +3.81 kB (+1.25%)
website/.docusaurus/routes.js 203 kB +1.28 kB (+0.63%)
website/.docusaurus/routesChunkNames.json 133 kB +3.27 kB (+2.52%)
website/build/assets/css/styles.********.css 113 kB +201 B (+0.18%)
website/build/assets/js/main.********.js 912 kB +5.36 kB (+0.59%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/codeTranslations.json 2 B
website/.docusaurus/globalData.json 123 kB
website/.docusaurus/i18n.json 930 B
website/.docusaurus/site-metadata.json 2.17 kB
website/build/index.html 38.1 kB

compressed-size-action

@OzakIOne
Copy link
Collaborator Author

OzakIOne commented Jun 20, 2024

Not sure about this poc refactor 5c789b3 (#10216)
It has some side effects on the tests in index.test.ts

lastUpdatedAt: undefined,
lastUpdatedBy: undefined,

The filter of the page author is now done in getPageAuthor instead of normalizeAuthor

So now we normalize all authors, and for the pageAuthor, we get only the authors with a generatePage === true

Is it better like this or the way before ? 🤷

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

First review pass

packages/docusaurus-utils/src/authors.ts Outdated Show resolved Hide resolved
packages/docusaurus-utils/src/authors.ts Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/options.ts Outdated Show resolved Hide resolved
}

export default function AuthorsListByLetter({authors}: Props): JSX.Element {
const letterList = listAuthorsByLetters(authors);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should group authors by letters. We should probably ask the community but it's also possible to list authors as a simple alphabetical line.

Similar to:
https://react.dev/community/team

To me it's unlikely authors will grow as fast as tags. But both options are valid, we can keep it this way for now.

Also: how do we handle authors that have no name? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should group authors by letters.

It would look like this:

To me it's unlikely authors will grow as fast as tags.

In an historical site like mine, it is highly likely that each new contribution will come from a new author.

how do we handle authors that have no name

Good question, it gave me quite some headaches; in the end I added a new object, NamedAuthors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ilg-ul we can't take your site as an example for this feature because we are building a blog plugin, not a chronological site. We enable you to render the list the way you want with swizzling, but it doesn't mean it's what we should provide by default for people that are actually building blogs.

However I'm fine with the current UI, it's not awesome but good enough to ship:
https://deploy-preview-10216--docusaurus-2.netlify.app/blog/authors

CleanShot 2024-06-21 at 17 55 58

Important to note that we only list authors on this page coming from authors.yml. Inline authors won't appear there.

Copy link
Contributor

Choose a reason for hiding this comment

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

The look is not relevant; the functionality seems equivalent (even better), so no problems so far.

Also no problems to ignore inline authors.

};

function getAuthorLetter(author: string): string {
return author[0]!.toUpperCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about authors without a name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now authors without names are named internaly with their key

const name = author.name || key;

So the route and the list of authors by letters are done with the key

Comment on lines 27 to 42
function useBlogPostsPlural() {
const {selectMessage} = usePluralForm();
return (count: number) =>
selectMessage(
count,
translate(
{
id: 'theme.blog.post.plurals',
description:
'Pluralized label for "{count} posts". Use as much plural forms (separated by "|") as your language support (see https://www.unicode.org/cldr/cldr-aux/charts/34/supplemental/language_plural_rules.html)',
message: 'One post|{count} posts',
},
{count},
),
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicated, need to be factorized. Since it's not related to styling, it can be moved to theme-common (same for all translation methods btw)

packages/docusaurus-theme-common/src/utils/authorsUtils.ts Outdated Show resolved Hide resolved
title: Joi.string(),
email: Joi.string(),
page: Joi.alternatives(Joi.bool(), AuthorPageSchema),
permalink: Joi.string(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We allow bool or AuthorPageSchema, however the type definition of page is null or AuthorPageSchema, tests throws page should be boolean or AuthorPage, else TS yells page to be null

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

Successfully merging this pull request may close these issues.

RFC: Blog authors pages
4 participants