feat(theme-common, theme-classic): Improve CodeBlock Extensibility #11011
Conversation
|
Hi @Danielku15! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
⚡️ Lighthouse report for the deploy preview of this PR
|
Danielku15
force-pushed
the
feature/codeblock-extensibility
branch
from
bbfcf38 to
ee22ed9
Compare
slorber
added
the
Argos
packages/docusaurus-theme-classic/src/theme/CodeBlock/Content/String.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
This PR is already quite large and hard to review
It would be better to split it into many subparts. We don't have to merge everything at once, incremental improvements are easier to merge.
@slorber
I wouldn't consider it "large", but it indeed has a bit of complexity to it. 😅 I'll split it up and refactor some bits as mentioned in the remarks.
I added some comments to give insights in my decision paths. No need to give again anwers, but it might contain some valueable bits for the split PRs.
packages/docusaurus-theme-classic/src/theme/CodeBlock/Content/String.tsx
Show resolved
Hide resolved
Even if it's not a huge PR, it still can be split into many smaller ones. I'd prefer to progress incrementally. Start by refactoring the code, without changing any behavior. Then apply fine-grained behavior changes, and justify each of them in a dedicated PR. If you refactor and change everything at once, then we may not agree on everything, and your PR is less likely to be merged fast. Let's start by focusing on what we agree on:
|
|
I fully agree on your points. Will work on adapted PRs soon-ish (just wrapping up some other general Docs parts on my project) 😉 |
|
All new PRs with individual changes are ready. They build on-top of each other for simplicity but if we decide to rework/not-merge some parts I'll rebase the changes accordingly . I also went through all remarks and I think they are covered in the new implementations. Lets continue the discussion step-by-step on the individual PRs. 😉 |
Pre-flight checklist
Motivation
Closes #11008
Test Plan
Verified and tested via unit tests. I primarily focused on
codeBlockUtils.test.tsbut all other tests locally are green. Looking at the rendered codeblocks on the website they are equal to before.Test links
Deploy preview: https://deploy-preview-11011--docusaurus-2.netlify.app/
Relevant Links:
Related issues/PRs
#11008