Skip to content

feat: Copy code button #8995

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

Merged
merged 19 commits into from
Sep 16, 2023
Merged

feat: Copy code button #8995

merged 19 commits into from
Sep 16, 2023

Conversation

PuruVJ
Copy link
Collaborator

@PuruVJ PuruVJ commented Jul 18, 2023

Related sveltejs/site-kit#175

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

⚠️ No Changeset found

Latest commit: 7e51773

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@PuruVJ PuruVJ marked this pull request as ready for review July 22, 2023 09:15
@PuruVJ PuruVJ requested a review from dummdidumm July 22, 2023 09:15
@dummdidumm
Copy link
Member

Is there a way to make this not apply to some code snippets? For example here
image
it's kinda useless to have the copy button on the one-liners, and it also disrupts the reading flow a bit. In general many code snippets are kind of incomplete, so I'm wondering what exactly the value proposition is for having a copy button on them. I'm wondering if there should be some kind of pragma in the docs to mark a code snippet as being full fledged enough that a copy snippet is warranted, for example by giving them a file name, which means they get a heading like some snippets in the Kit docs, and then there's space already to the right where the copy code button can be placed.

@dummdidumm
Copy link
Member

I don't know why, but it seems the file logic isn't working for some snippets: https://svelte-gz1whcs22-svelte.vercel.app/docs/svelte-components#script . Could you investigate?

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 4, 2023

Happening cuz of cache on vercel. The script adds a class copy-code-block to copyable code snippets, which is stored in the cache and targeted on runtime. Redeploying without cache will fix it

EDIT: Rebuilt without cache, check again https://svelte-dev-git-feat-copy-code-button-svelte.vercel.app/docs/svelte-components

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 4, 2023

Good to go from my side!

@Rich-Harris
Copy link
Member

I feel like the file names add a lot of visual noise that I'm not sure is justified by this change. (To be honest I didn't realise how prevalent file names were in the docs already — if it were up to me we'd get rid of them. Can we?)

I also think we can make these buttons a bit more subtle — smaller and less opaque. Something like this?

image

@dummdidumm
Copy link
Member

The file names were my idea, as I felt that having copy buttons everywhere was adding visual clutter, and given we already have these in SvelteKit, we might aswell add it to th Svelte core docs and then add the copy button on the right.

More opaque copy code button would be good either way I think, I'd say visual clarity should outweigh accessibility concerns here, especially since this is not a critical part of the app.

@dummdidumm
Copy link
Member

why are these comments inside the code in the first place, can't we just add this everywhere with the code fence renderer?

Because then it would be applied to one-liners or fractions of code snippets, too, where it doesn't make sense and just creates visual clutter. It was that way initially and it looked bad, see this comment: #8995 (comment)

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 7, 2023

I believe Dominik meant why it is: /// copy: true instead of being next to ```svelte {copy}

@dominikg
Copy link
Member

dominikg commented Aug 7, 2023

full line fences add "visual clutter" already, the copy button in itself doesn't change that much, esp if it is the less dominant variation Rich suggested.

And who are we to judge what the user wants to copy or not. having these copy buttons only in some places adds inconsistencies too, which isn't great for UX either.

@dummdidumm
Copy link
Member

The snippets in that picture are no real code snippets, they are part of the API definition. So they shouldn't look like code snippets which a copy button makes them look like more.

@dominikg
Copy link
Member

dominikg commented Aug 7, 2023

looking at https://svelte.dev/docs/svelte-transition i can understand why you are afraid of adding more visual noise.

There are no less than 4 different visual appearances of code

  • inline
  • multi-line block for api definition (generated from a markdown quote, slighly different bg color)
  • single line block without color highlight
  • multi-line block for an example

Important links like the related tutorial section and the transition directive these are based off are not standing out against simple type definition links.

So i agree adding copy buttons on every block of that page would make the situation even worse. But the solution isn't littering the rest of the docs with opt-in syntax to show a copy button. You can either add an opt-out there - or - which would be my preferred approach, redo these sections to be more approachable, have less code blocks and then having a copy button on them suddenly isn't noise anymore but the helpful feature we want it to be...

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 7, 2023

We can set copy: true by default for every snippet and add copy: false to the definitions ones

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 9, 2023

Hey folks, what's the verdict on this? Do I move forward with reverting file: change, making copy: true globally and putting copy: false on these API snippets?

@dominikg
Copy link
Member

sounds like an initial plan to me

Hey folks, what's the verdict on this? Do I move forward with reverting file: change, making copy: true globally and putting copy: false on these API snippets?

sounds like a plan, i'd add a slight update on the copy button style so it's a bit more mute/neutra

how much work would the pragma thing be though?

slightly related for code block rendering: https://github.com/antfu/shikiji

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Aug 26, 2023

how much work would the pragma thing be though?

WDYM? Changing it from /// copy: true to {copy:true}? Or setting it to true for all and specifying false for certain snippets?

slightly related for code block rendering: https://github.com/antfu/shikiji

Ohh this looks good. Although shiki-twoslash uses shiki internally. I wonder if there's a way to swap shiki with shikiji in that department 🤔

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Sep 16, 2023

Applied the discussed changes.

  • copy: true by default
  • file: also gets copy button
  • Applied copy: false to the snippets which show the syntax(directly under headings, like under transition: heading)

@PuruVJ PuruVJ merged commit 1a28d58 into master Sep 16, 2023
@PuruVJ PuruVJ deleted the feat/copy-code-button branch September 16, 2023 14:30
kelvinsjk pushed a commit to kelvinsjk/svelte that referenced this pull request Oct 19, 2023
* Push

* Bump site-kit

* Add headers to primary snippets

* Update deps

* Bump deos

* redploy

* Back to normal

* Push

* Bump deps

* site: fix rendering of promise in deprecation warning (sveltejs#9191)

* copy: true

* Bump site-kit

* Use cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants