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(theme-classic): allow collapsing part of code blocks #5783

Closed
wants to merge 18 commits into from

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Oct 25, 2021

Motivation

Resolve #2215. I need this because most of the config tutorials on the website contain a lot of boilerplate to provide the context, but collapsing them makes the doc much more concise.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview: https://deploy-preview-5783--docusaurus-2.netlify.app/docs/next/sidebar/#collapsible-categories

Test

TODO

  • Collapsing animation (using <Collapsible>)
  • Better design (button color; icon; a boundary between collapsible parts and non-collapsible ones)
  • Add a config option for default collapsed state?
  • Write docs
  • More usage on the website

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 25, 2021
@Josh-Cena Josh-Cena marked this pull request as draft October 25, 2021 12:07
@netlify
Copy link

netlify bot commented Oct 25, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: f8e226c

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

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

@github-actions
Copy link

github-actions bot commented Oct 25, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 87
🟢 Accessibility 98
🟢 Best practices 93
🟢 SEO 100
🟢 PWA 92

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

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Oct 28, 2021
@Josh-Cena Josh-Cena marked this pull request as ready for review December 3, 2021 14:00
@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 5, 2021
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.

I find this feature interesting and we could definitively use something like this, but it's probably not so easy to get right, and not a high priority either

Some concerns:

  • Does it really need to be in core? Maybe we can instead think of a more generic API so that users can build this kind of feature in userland, inventing their own markers? => smaller and more flexible API surface => less to maintain

  • Inability to mix top-level highlight with sample markers

  • Current UX is unable to contain multiple samples. GitHub diff/PR UX may be better?

  • How does it work with progressive enhancement in mind and JS disabled? (may not matter much, as the meat of the content remains available)

@@ -148,6 +153,14 @@ export function parseLines(
highlightRange += `${highlightBlockStart!}-${lineNumber - 1},`;
break;

case 'sample-start':
Copy link
Collaborator

Choose a reason for hiding this comment

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

wonder if sample is the best marker name? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is directly from the Kotlin docs example in #2215: // sampleStart in that case. We can certainly bikeshed about this, and if we are to just build a generic API, maybe even no bikeshedding at all :D

@Josh-Cena
Copy link
Collaborator Author

Does it really need to be in core? Maybe we can instead think of a more generic API so that users can build this kind of feature in userland, inventing their own markers? => smaller and more flexible API surface => less to maintain

Sure, now we are back to the question of allowing more markers 👍 I'll look into making parseLines more generic & extensible.

@Josh-Cena Josh-Cena added status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. and removed status: awaiting review This PR is ready for review, will be merged after maintainers' approval labels Dec 19, 2021
@Josh-Cena
Copy link
Collaborator Author

Now that #7178 is merged, I will be closing this, because I feel like this is better implemented in userland, since different people will have ideas about different UX with such complicated feature. The linked issue will be closed as well.

cc @hamza1311 I know Yew will benefit from this, but feel free to swizzle CodeBlock and port the changes in this PR there.

@Josh-Cena Josh-Cena closed this May 4, 2022
@Josh-Cena Josh-Cena deleted the collapse-codeblock branch May 4, 2022 10:34
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: new feature This PR adds a new API or behavior. status: don't merge yet This PR is completed, but we are still waiting for other things to be ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Folding sections in code blocks
3 participants