-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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): author header social icons #10222
base: main
Are you sure you want to change the base?
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Size Change: +83 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
const link = url || (email && `mailto:${email}`) || undefined; | ||
const renderSocialMedia = () => ( | ||
<div className={clsx(styles.authorSocial)}> | ||
{(twitter as string) && <Twitter user={twitter as string} />} |
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.
unknown keys such as twitter
and github
(and other keys that are not predefined) are typed as unknown which triggers TS to yell Type 'unknown' is not assignable to type 'ReactNode'
However as string
might be an ugly solution, otherwise maybe update the Props
type in theme classic.d.ts
but which key should we define here ? (twitter?github? other random website?)
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.
We shouldn't use unknown keys in the first place
The type should contain a new "socials" attribute and we should validate those socials for both authors.yml and inline authors front matter
We should cover the most common socials (github, twitter/x, LinkedIn, SO, IG, website...) and allow unknown props (.unknown()
in schema, [key: string]: string
in type) in case users want to add extra social platforms
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.
I've changed the schema to allow a social object, however i'm not sure about the typing of unknown props if the user add a social website
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
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.
Not sure how these social icons should display yet, but we need to discuss that.
We'd need many dogfood examples here to review, covering many distinct blog post situations: https://deploy-preview-10222--docusaurus-2.netlify.app/tests/blog/
One concern I have using React SVG components meant to be repeated on a page is the static markup duplication. It could be more optimized to use external SVGs, but let's figure that out later (see #5865)
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
<a | ||
href={twitterUrl} | ||
target="_blank" | ||
rel="noreferrer noopener" | ||
className={clsx(styles.socialIcon)}> |
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.
Extract AuthorSocialLink
component, icon should be received as children
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
const link = url || (email && `mailto:${email}`) || undefined; | ||
const renderSocialMedia = () => ( | ||
<div className={clsx(styles.authorSocial)}> | ||
{(twitter as string) && <Twitter user={twitter as string} />} |
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.
We shouldn't use unknown keys in the first place
The type should contain a new "socials" attribute and we should validate those socials for both authors.yml and inline authors front matter
We should cover the most common socials (github, twitter/x, LinkedIn, SO, IG, website...) and allow unknown props (.unknown()
in schema, [key: string]: string
in type) in case users want to add extra social platforms
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
{hasSocialMedia ? ( | ||
renderSocialMedia() | ||
) : ( | ||
<small className="avatar__subtitle">{title}</small> | ||
)} |
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.
I'm not sure we should choose one or the other, but let's discuss and figure this out
packages/docusaurus-theme-classic/src/theme/BlogPostItem/Header/Author/index.tsx
Outdated
Show resolved
Hide resolved
|
||
// SVG Source: https://svgl.app/ | ||
function Github(props: SVGProps<SVGSVGElement>): JSX.Element { | ||
const {colorMode} = useColorMode(); |
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.
Don't use this, hydration error
Pre-flight checklist
Motivation
Prepare for blog author page and also community request #10136
Also which UI UX should we adopt for the social icons ?
Visual mockup
Test Plan
Preview
add dogfood tests
Test links
https://deploy-preview-10222--docusaurus-2.netlify.app/blog/
Related issues/PRs
Fix #10136