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.

Sign up for GitHub

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

Jump to bottom

refactor: better typing + remove unnecessary eslint-disable #5335

Merged
merged 13 commits into from
Aug 11, 2021
Merged

refactor: better typing + remove unnecessary eslint-disable #5335

merged 13 commits into from
Aug 11, 2021

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

  • Fix TS build errors
  • Less use of any
  • Better type hints when using useDocusaurusContext

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Build & test

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 11, 2021
@netlify
Copy link

netlify bot commented Aug 11, 2021 •
edited
Loading

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 1249923

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6113c88ba8f5c50008b1dbb4

😎 Browse the preview: https://deploy-preview-5335--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Aug 11, 2021 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 55
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@Josh-Cena
Initial work
3547eca 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Fix a few errors
1e64599 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Restore default value
6bad04e 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Update docs
06fe5d7 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Use custom route config
7e2192e 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
slorber
slorber requested changes Aug 11, 2021
View reviewed changes
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.

Thanks!

Didn't notice at first you modified the bootstrap theme: ignore my comments about it, we don't care much about maintaining it atm, we might as well simply deprecate it because it's not in an usable state currently

packages/docusaurus-theme-bootstrap/src/theme/DocItem/index.tsx Show resolved Hide resolved
packages/docusaurus-theme-bootstrap/src/theme/Footer/index.tsx Show resolved Hide resolved
packages/docusaurus-theme-classic/src/theme/DocPage/index.tsx
@@ -57,7 +55,6 @@ function DocPageContent({

return (
<Layout
key={isClient}
Copy link
Collaborator

Choose a reason for hiding this comment

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

agree, this one has always been a bit suspicious to me and don't think it should be needed. It may have side-effects though, that we may have to handle in a different way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just realized that in CodeBlock we also have key={String(mounted)}. Maybe this is some black magic on client render?

Copy link
Collaborator

Choose a reason for hiding this comment

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

just keep the keys for now, we'll take a look at this in another PR. The key in CodeBlock seems there for a good reason, but this one can probably be removed

packages/docusaurus-theme-classic/src/theme/Logo/index.tsx
} = useDocusaurusContext();
const {
navbar: {title: navbarTitle, logo = {src: ''}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

never linked too much this comp 🤪 a bit messy

maybe a good time to extract a separate one and cleanup a bit the code? I'm thinking of something like:

      {logo && <LogoThemedImage logo={logo}/>)}

Copy link
Collaborator Author

@Josh-Cena Josh-Cena Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This seems a bit hard, since the use of logo is intertwined with rendering logic of the component itself. Should we use Joi instead to make a default logo: {src: ''}?

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree, not so easy 😅 I'll look at simplifying this in another pr

packages/docusaurus-theme-classic/src/theme/NavbarItem/LocaleDropdownNavbarItem.tsx Outdated Show resolved Hide resolved
packages/docusaurus-theme-common/src/utils/generalUtils.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/client/baseUrlIssueBanner/BaseUrlIssueBanner.tsx Outdated Show resolved Hide resolved
@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Aug 11, 2021 •
edited
Loading

we might as well simply deprecate it because it's not in an usable state currently

Yes, I find it a bit painful to backport the changes for type checks to pass while it seems so unmaintained... It seems to have zero usage on NPM. I was just making minimal changes to make TSC happy

@Josh-Cena
Address a few suggestions
942d6a4 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Revert logo change
8d5a542 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Restore key
899bb1a 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Oops
8721ac0 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Remove use of any
f390b27 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Remove eslint-disable
1056c20 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena changed the title refactor: properly type some client modules refactor: properly type some client modules + remove unnecessary eslint-disable Aug 11, 2021
@Josh-Cena
Put type reference back
b0d2e58 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena changed the title refactor: properly type some client modules + remove unnecessary eslint-disable refactor: better typing + remove unnecessary eslint-disable Aug 11, 2021
@Josh-Cena
Remove
1249923 
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@slorber
Copy link
Collaborator

slorber commented Aug 11, 2021

LGTM thanks 👍 we can do other changes in subsequent PRs

@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Aug 11, 2021
@slorber slorber merged commit ee6ebc4 into facebook:master Aug 11, 2021
@Josh-Cena Josh-Cena deleted the type-context branch August 11, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber requested changes

@lex111 lex111 Awaiting requested review from lex111

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Josh-Cena @slorber @facebook-github-bot