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(v2): allow any type of NavbarItem to be passed in dropdown #5072

Merged
merged 43 commits into from Jul 14, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Jun 27, 2021

Motivation

  1. Fix eslint error (because of the any type).
  2. Fix Docs link in navbar dropdowns are completely broken #5071. Resolve Navbar dropdown items should support type "doc" #4996.
  3. Algorithmic refactors

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The website should be able to test most use cases; how do I test other cases? (e.g. when the dropdown items are doc links)

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 27, 2021
@Josh-Cena Josh-Cena marked this pull request as draft June 27, 2021 15:09
@Josh-Cena Josh-Cena changed the title refactor(v2): make NavbarItem polymorphic [WIP] refactor(v2): make NavbarItem polymorphic Jun 27, 2021
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena changed the title [WIP] refactor(v2): make NavbarItem polymorphic [WIP] refactor(v2): correct NavbarItem logic Jun 28, 2021
@netlify
Copy link

netlify bot commented Jun 28, 2021

✔️ [V2]

🔨 Explore the source changes: c12f0ba

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

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

@github-actions
Copy link

github-actions bot commented Jun 28, 2021

⚡️ Lighthouse report for the changes in this PR:

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

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

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena changed the title [WIP] refactor(v2): correct NavbarItem logic feat(v2): allow any type of NavbarItem to be passed in dropdown Jun 28, 2021
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena
Copy link
Collaborator Author

@slorber I'm mostly done now. Should I take care of the joi / docs, or can we merge this and worry about other issues later 🤷‍♂️

@slorber
Copy link
Collaborator

slorber commented Jul 9, 2021

In any case I want to merge this PR #4273 asap before this one.

I'll update your PR and resolve conflicts if needed and then will review this one. Perfect validation can be taken care of later

Josh-Cena and others added 8 commits July 10, 2021 14:58
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
# Conflicts:
#	packages/docusaurus-theme-classic/src/theme/NavbarItem/DefaultNavbarItem.tsx
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, that looks much cleaner to have a separate dropdown component, wanted to do that as well :)

We'll figure out the types/validation later


function getDocInVersions(versions: GlobalDataVersion[], docId: string) {
// vanilla-js flatten, TODO replace soon by ES flat() / flatMap()
const allDocs: GlobalDataDoc[] = [].concat(
Copy link
Collaborator

Choose a reason for hiding this comment

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

not 100% sure but I think we need to keep it for older node/browsers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is taken care of by Babel, as long as the target is set to ES5 or ES6?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes babel probably does something with browserslist

I think I avoided flatMap in the past due to having to support node10

} & ComponentProps<'a'>;
import type {LinkProps} from '@docusaurus/Link';

export type NavLinkProps = LinkProps & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keeping LinkProps here?

@@ -1,7 +1,7 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"lib": ["DOM"],
"lib": ["DOM", "ESNext"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

replacing this with ES2019, as it will run in NodeJS 12 (unlike any new ES2020 feature)

However not yet 100% convinced it's the right move because some ES2019/older features don't have all a very good browser support, so we'll have to remember to not use them

@slorber
Copy link
Collaborator

slorber commented Jul 14, 2021

Thanks, going to merge this.

We'll use later validation to ensure that we don't try to render a recursive dropdown structure etc because currently this is
possible 🤪 (even if user is unlikely to try this as it does not make sense)

image

Also thinking of forcing users to provide a type: "dropdown" in the future to avoid this dynamic/implicit type behavior. Probably by first adding a warning when not using the dropdown type and items array exist

@slorber slorber merged commit 007e901 into facebook:master Jul 14, 2021
@Josh-Cena Josh-Cena deleted the navbaritem branch July 15, 2021 00:55
@slorber
Copy link
Collaborator

slorber commented Jul 15, 2021

I don't know, no strong feelins about this yet but in general I prefer a stricter config (ie all items have an explicit type, including link/dropdown) than having the config be implicitly typed. This is also simpler for our own code (ex the validation does not have to be dynamic)

@Josh-Cena
Copy link
Collaborator Author

Oops, not expecting for such a quick response 😂 In any case let's discuss in the docs PR

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.

Docs link in navbar dropdowns are completely broken Navbar dropdown items should support type "doc"
3 participants