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

fix(theme-classic) extract HomeBreadcrumbItem + fix swizzle bugs #8445

Merged
merged 8 commits into from Dec 29, 2022
Merged

fix(theme-classic) extract HomeBreadcrumbItem + fix swizzle bugs #8445

merged 8 commits into from Dec 29, 2022

Conversation

3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Dec 15, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • 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

See #6953 (comment)

Test Plan

1.

yarn test (in the repo root) is green

2.

I changed website/package.json:

-     "@docusaurus/theme-classic": "^3.0.0-alpha.0",
+     "@docusaurus/theme-classic": "file:../packages/docusaurus-theme-classic",

and then

cd website
yarn install
npm run swizzle @docusaurus/theme-classic DocBreadcrumbs/HomeBreadcrumbItem -- --eject

Notice that swizzling produces the following warning:

[WARNING]
Swizzle action eject is unsafe to perform on DocBreadcrumbs/HomeBreadcrumbItem.
It is more likely to be affected by breaking changes in the future
If you want to swizzle it, use the `--danger` flag, or confirm that you understand the risks.

? Do you really want to swizzle this unsafe internal component? › - Use arrow-keys. Return to submit.
    NO: cancel and stay safe
❯   YES: I know what I am doing!
    [Exit]

I also changed the icon in the .tsx

<Link ...>
  <IconHome className={styles.breadcrumbHomeIcon} />
  HOME SWEET HOME <-----
</Link>

and .css

.breadcrumbHomeIcon {
  position: relative;
  top: 1px;
  vertical-align: top;
  height: 1.1rem;
  width: 1.1rem;
  color: red; <-----
}

which resulted in

Red home icon and HOME SWEET HOME on the right

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

See Motivation above.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 15, 2022
@netlify
Copy link

netlify bot commented Dec 15, 2022

[V2]

Name Link
🔨 Latest commit 41e90b6
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63ad87d8366bee000870bd7a
😎 Deploy Preview https://deploy-preview-8445--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 settings.

@github-actions
Copy link

github-actions bot commented Dec 15, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 65 🟢 97 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 77 🟢 100 🟢 100 🟢 100 🟢 90 Report

@3v0k4
Copy link
Contributor Author

3v0k4 commented Dec 15, 2022

Is styles.breadcrumbsItemLink unused code that we can remove?

@3v0k4 3v0k4 marked this pull request as ready for review December 15, 2022 15:50
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 👍

See PR comments

CleanShot 2022-12-16 at 16 55 05@2x

No need to, monorepos hoist and use local packages ;)

@3v0k4
Copy link
Contributor Author

3v0k4 commented Dec 16, 2022

See PR comments

No need to, monorepos hoist and use local packages ;)

TIL 🙏

@3v0k4 3v0k4 requested review from slorber and removed request for lex111 and Josh-Cena December 16, 2022 19:12
@slorber slorber added pr: polish This PR adds a very minor behavior improvement that users will enjoy. to backport This PR is planned to be backported to a stable version of Docusaurus labels Dec 22, 2022
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 👍 LGTM

@slorber
Copy link
Collaborator

slorber commented Dec 22, 2022

hmmm, little CI failure that I will have to check, not your fault :)

@slorber slorber changed the title refactor(theme-classic) extract HomeBreadcrumbItem (#6953) refactor(theme-classic) extract HomeBreadcrumbItem + fix swizzle bugs Dec 29, 2022
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Dec 29, 2022
@slorber slorber changed the title refactor(theme-classic) extract HomeBreadcrumbItem + fix swizzle bugs fix(theme-classic) extract HomeBreadcrumbItem + fix swizzle bugs Dec 29, 2022
@slorber slorber merged commit e1d6292 into facebook:main Dec 29, 2022
@3v0k4 3v0k4 deleted the eject-home-breadcrumb-item branch December 29, 2022 20:48
slorber added a commit that referenced this pull request Jan 26, 2023
Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
@slorber slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release. 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.

None yet

3 participants