Skip to content

refactor(v2): add missing theme-classic types#4385

Merged
slorber merged 1 commit into
facebook:masterfrom
armano2:fix/theme-types
Mar 12, 2021
Merged

refactor(v2): add missing theme-classic types#4385
slorber merged 1 commit into
facebook:masterfrom
armano2:fix/theme-types

Conversation

@armano2

@armano2 armano2 commented Mar 10, 2021

Copy link
Copy Markdown
Contributor

Motivation

Improve types for theme-classic

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

if compilation passes everything is fine

Related tickets

ref; #3424

@armano2
armano2 requested review from lex111 and slorber as code owners March 10, 2021 20:11
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 10, 2021
@netlify

netlify Bot commented Mar 10, 2021

Copy link
Copy Markdown

@netlify

netlify Bot commented Mar 10, 2021

Copy link
Copy Markdown

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 23eada4

https://deploy-preview-4385--docusaurus-2.netlify.app

@github-actions

github-actions Bot commented Mar 10, 2021

Copy link
Copy Markdown

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 84
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4385--docusaurus-2.netlify.app/classic/

}

declare module '@theme/LastUpdated' {
export type Props = {

@armano2 armano2 Mar 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i'm unsure if using Props as a type everywhere is a good idea, but in 90% of cases this name is used.

i was tempted to rename all of them to {ComponentName}Props but this will be a bigger change, if you think that's a good idea i can do it in separate PR or update this one

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

don't know

Honestly not very satisfied with our current TS setup, this is a bit annoying to maintain and I'd rather emit types based on our actual code rather than hand-write it as we do today.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok i'm going to prepare PR for that :)

@slorber slorber added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 12, 2021

@slorber slorber left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks :) LGTM

}

declare module '@theme/LastUpdated' {
export type Props = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

don't know

Honestly not very satisfied with our current TS setup, this is a bit annoying to maintain and I'd rather emit types based on our actual code rather than hand-write it as we do today.

@slorber slorber changed the title fix(v2): add missing theme-classic types refactor(v2): add missing theme-classic types Mar 12, 2021
@slorber
slorber merged commit a39c62f into facebook:master Mar 12, 2021
@slorber

slorber commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

Thanks :)

Note if you are interested to help us complete our TS types, we have this theme config that is currently very badly typed 😅
https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-theme-common/src/utils/useThemeConfig.ts

@armano2
armano2 deleted the fix/theme-types branch March 12, 2021 15:17
@armano2

armano2 commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

@slorber i started working on configs already, but i think its better to do this in smaller batches as they are easier to review

@slorber

slorber commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

yes definitively :) thanks

This was referenced Mar 17, 2021
mrizwanashiq pushed a commit to mrizwanashiq/docusaurus that referenced this pull request Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants