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

Use diff syntax for codeblocks #2984

Closed
wants to merge 2 commits into from
Closed

Use diff syntax for codeblocks #2984

wants to merge 2 commits into from

Conversation

cortinico
Copy link
Contributor

I've updated several codeblocks in the migration guide to use the diff syntax + I've added several missing title='...' so it's cleared of which file we're talking about.

@netlify
Copy link

netlify bot commented Mar 1, 2022

✔️ Deploy Preview for react-native ready!

🔨 Explore the source changes: ac2ad82

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-native/deploys/621e63c5ba2050000807f08f

😎 Browse the preview: https://deploy-preview-2984--react-native.netlify.app

@netlify
Copy link

netlify bot commented Mar 1, 2022

✔️ Deploy Preview for react-native ready!

🔨 Explore the source changes: 558df33

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-native/deploys/621e63d0645219000788ac32

😎 Browse the preview: https://deploy-preview-2984--react-native.netlify.app

@cortinico cortinico requested a review from Simek March 3, 2022 11:38
Copy link
Collaborator

@Simek Simek left a comment

Choose a reason for hiding this comment

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

Most of the changes looks good, thank you for working on this! 👍

However I don't like that much, how the diff highlight works. I see two problems with it:

  • manually selecting and copying lines or using "Copy" button now includes + or - chars for diff lines, which had to be manually removed after paste,
  • the language based code highlight do not work in diff blocks (maybe this can be added somehow?), which leads to decreased readability of the larger example code blocks.

Unfortunately there is no easy solution to fix those issues, and there are not many viable alternatives (there is // highlight-start/end but it's just a visual lines highlight, no ability to alter the appearance or leave a comment - https://docusaurus.io/docs/next/markdown-features/code-blocks#line-highlighting), so I think it's fine to merge it as is for now, but we should investigate, how we can improve diff code block rendering in the future. Also, maybe part of this can be addressed directly via PR to Docusaurus, I will look into that.

@cortinico
Copy link
Contributor Author

cortinico commented Mar 3, 2022

which had to be manually removed after paste,

Yup I noticed the same. What is worse is that also selecting multiple lines carries over the +.

(there is // highlight-start/end but it's just a visual lines highlight, no ability to alter the appearance or leave a comment - docusaurus.io/docs/next/markdown-features/code-blocks#line-highlighting)

Maybe I can update those diff which are involving more than say 5 lines, to use this feature instead, wdyt?

Also, maybe part of this can be addressed directly via PR to Docusaurus, I will look into that.

And yes, if we fix this, we should definitely backport them upstream

@Simek
Copy link
Collaborator

Simek commented Mar 3, 2022

It looks like Prism supports diff highlight with language highlight included via plugin, not sure if react-prism-renderer, which is used in Docusaurus under the hood supports that:

Maybe I can update those diff which are involving more than say 5 lines, to use this feature instead, wdyt?

I have no strong opinion about this, we can leave the diff blocks as is or later the bigger ones as you suggested. I think if we want to try improve this in the future, it would be nice to avoid the work which will be reverted if we succeed. Also, in the other hand using diffs there also improves the examples i na way that it shifts the focus to the important part of the snippets.

@slorber
Copy link
Contributor

slorber commented Mar 4, 2022

On the docusaurus side, we'd definitively be interested to be able to:

  • have syntax highlighting on diff codeblocks
  • make it easier to copy working code from diff

We had a few issues in our own tutorials where users suggested to move back from diff to regular code block:

cc @Josh-Cena

@Josh-Cena
Copy link

have syntax highlighting on diff codeblocks

This can be done with a Prism plugin. I tried a while back by swizzling prism-include-languages but no luck, and didn't bother making it work.

make it easier to copy working code from diff

This would be done through a string sanitization before passing to copyToClipboard, but last time when facebook/docusaurus#4698 was brought up, we decided that if a diff code block needs to be copied, it should probably be a normal code block instead. The valid use-cases of diff are those that are purely illustrative, like https://docusaurus.io/docs/sidebar/autogenerated#using-number-prefixes

Not sure if we necessarily want to handle diff specially—should we copy the - lines, or only the + lines? I mean, the entire purpose of diff is to show a process, not a result...

@slorber
Copy link
Contributor

slorber commented Mar 4, 2022

Not sure if we necessarily want to handle diff specially—should we copy the - lines, or only the + lines? I mean, the entire purpose of diff is to show a process, not a result...

Somehow agree but users expect this to work apparently. Also there's a copy button on :hover so it's a bit misleading. Maybe we should just remove that copy button for diff code blocks?

@Josh-Cena
Copy link

Josh-Cena commented Mar 4, 2022

Maybe we should just remove that copy button for diff code blocks?

Doesn't sound like a bad idea.

@cortinico
Copy link
Contributor Author

What's the final decision here @Simek ? How do you feel about using the diff syntax only to changes of 3/4 lines?

@Simek
Copy link
Collaborator

Simek commented Apr 7, 2022

Sound good to me! 👍 It's not a blocker anyway, since there is no ideal solution yet.

P.S. Unfortunately I was not able to look at the Prisma plugin port/addition yet, but I have this in my schedule for this month.

@Josh-Cena
Copy link

@Simek Recently I've come to known that the Prism plugin actually won't work with prism-react-renderer which Docusaurus uses. See FormidableLabs/prism-react-renderer#90

@Josh-Cena
Copy link

I've recently set this up for the TS-ESLint website: https://deploy-preview-5099--typescript-eslint.netlify.app/docs/linting/monorepo#one-tsconfigjson-per-package-and-an-optional-one-in-the-root

Had to swizzle CodeBlock to achieve the copy button customization, not sure if we can do better in Docusaurus

@Simek
Copy link
Collaborator

Simek commented May 31, 2022

Thanks for the hint, I will look at the code in there soon and try to port it into RN website.

@Josh-Cena
Copy link

Josh-Cena commented May 31, 2022

I think Docusaurus can make the swizzling a bit less heavy here: e.g. we can pass lineClassNames into CopyButton so that we only need to swizzle CopyButton to change its behavior. @slorber WDYT?

@slorber
Copy link
Contributor

slorber commented Jun 1, 2022

@Josh-Cena as far as I understand you talk about this PR: https://github.com/typescript-eslint/typescript-eslint/pull/5099/files

Not against passing more things to <CopyButton> if that helps reduce the amount of swizzled code

However, maybe it would be better to make this more automatic in the end: using diff should just work 🤷‍♂️

@Josh-Cena
Copy link

IDK—I feel like this is quite opinionated and not necessarily correct, and it would be dangerous for us on a framework level to assume what users want when they use diff—what if they just want to illustrate, well, a diff?

In typescript-eslint/typescript-eslint#5099 I made the removed lines non-selectable and non-copyable, which suits that particular use-case, but is not a fair assumption to make.

@slorber
Copy link
Contributor

slorber commented Jun 2, 2022

@Josh-Cena I don't think many wouldn't want this behavior in practice

What if we made diff behavior opinionated automatically, but allow another metastring to opt-out?

Like diff preserve vs diff?

This requires to document it, but that seems fine

@Josh-Cena
Copy link

I'm ok with that, sure... If we can design it well.

@cortinico
Copy link
Contributor Author

I'm abandoning this. The docs changed so much that this is not relevant. We've been using the Diff blocks sporadically in the new docs + we reduce the amount of changes needed for the new arch so this is probably not relevant anymore.

@cortinico cortinico closed this Sep 13, 2022
@yungsters yungsters deleted the nc/diff-syntax branch December 22, 2022 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants