Skip to content

[docs-infra] Add header to editable code blocks#37648

Closed
gitstart wants to merge 6 commits intomui:masterfrom
GitStartHQ:MUI-37556
Closed

[docs-infra] Add header to editable code blocks#37648
gitstart wants to merge 6 commits intomui:masterfrom
GitStartHQ:MUI-37556

Conversation

@gitstart
Copy link
Contributor

Closes #37556


This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@mui-bot
Copy link

mui-bot commented Jun 19, 2023

Netlify deploy preview

https://deploy-preview-37648--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 3f194c7

@danilo-leal danilo-leal changed the title MUI-37556 - Copy button on components code snippet [docs] Adjust code snippet copy button positioning Jun 20, 2023
@danilo-leal danilo-leal added the docs Improvements or additions to the documentation. label Jun 20, 2023
@gitstart gitstart marked this pull request as ready for review June 22, 2023 09:20
@gitstart gitstart marked this pull request as draft June 22, 2023 09:21
gitstart and others added 2 commits June 28, 2023 16:56
@gitstart gitstart marked this pull request as ready for review June 29, 2023 08:50
@gitstart
Copy link
Contributor Author

@danilo-leal This is ready for review

@zannager zannager requested a review from michaldudak June 29, 2023 09:24
@danilo-leal danilo-leal changed the title [docs] Adjust code snippet copy button positioning [docs-infra] Add header to editable code blocks Jun 30, 2023
@danilo-leal
Copy link
Collaborator

@gitstart Nice, thanks for contributing ⎯ really appreciate it! I can't commit to this PR, though, am getting an authentication error for some reason, would you know why?

Screen Shot 2023-06-30 at 12 40 27

@gitstart
Copy link
Contributor Author

@gitstart Nice, thanks for contributing ⎯ really appreciate it! I can't commit to this PR, though, am getting an authentication error for some reason, would you know why?

Screen Shot 2023-06-30 at 12 40 27

Hi @danilo-leal, not sure what the issue could be, but you can state any adjustment you want to make, and I can implement it from my end.

Copy link
Collaborator

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Alrighty, send the changes I was about to push in the form of a review!

gitstart and others added 2 commits June 30, 2023 16:34
Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@gitstart
Copy link
Contributor Author

Alrighty, send the changes I was about to push in the form of a review!

Your changes have been implemented @danilo-leal

@danilo-leal
Copy link
Collaborator

That's great ⎯ thanks a lot, @gitstart! 🙏 However, through further reflection, even though it looks great, I'm starting to question whether this is a good thing to add to the code blocks. My train of thought is:

We thought about adding this header initially to solve the issue of the overlapping scrollbar with the "Copy" button. Adding the "Editable code" text was an extra thing to take advantage of the header and also communicate that better. Visitors currently learn that these blocks are editable by hovering them and seeing a thicker border and the cursor text indication. Given it's not incredibly difficult to hover over the code blocks even by accident and eventually end up learning that, I'm wondering if the added visual weight of more textual elements on the UI + the size increase of the larger demo container is ultimately worth it.

I also started to wonder if there's an alternative solution for the overlapping scrollbar with the Copy button that doesn't necessarily involve adding new elements to the code block. 🤔

cc @zanivan & @DavidCnoops wanna hear from y'all ⎯ what do you think about it?
For context, visit the issue that sparked this PR & the deploy preview.

@mapache-salvaje
Copy link
Contributor

I like the idea in theory but I think the execution needs a little work. This spacing looks a little wonky to me, for example:

Screenshot 2023-07-01 at 5 40 31 PM

@danilo-leal
Copy link
Collaborator

Yeah 😅 But wouldn't worry about it too much here as there's another PR (#37664) that's refining this button and even changing it to an icon button instead!

@alexfauquette
Copy link
Member

Adding the "Editable code" text was an extra thing to take advantage of the header and also communicate that better

Just a thought I had when reading the discussion: Would it be more interesting to highlight the expandable aspect of the code? If I compare the two aspects I've

  • Editable:
    • discoverability: Since devs are big copy/past users, they could edit by random with bad key press (for example Ctrl+X)
    • importance: Nice feature but you can leave without (for example with codesandbox)
  • Expansion:
    • discoverability: There is one button with an icon that does not seems to be obvious <>
    • importance: For complex components it's necessary. Without this feature you loose a big par of the disc experience

From time to time we get docs feedback such as

More information about how to actually write the code
sent from https://mui.com/x/react-date-pickers/date-range-calendar/#choose-the-months-to-render (from section Localization)

@danilo-leal
Copy link
Collaborator

danilo-leal commented Jul 3, 2023

Yeah, that makes sense to me! They don't sound like mutually exclusive problems, though ⎯ we could go after promoting both of these functionalities given how important they are.

@DavidCnoops
Copy link
Contributor

I quite like this solution. The 'editable code' copy in the header is IMO definitely useful, as it's otherwise not immediately apparent the box is in fact editable. I'd also keep the 'copy' button visible at all times (i.e. not only when the box is hovered) as it makes the copy functionality more discoverable and its slightly more convenient to quickly copy something this way (you don't have to first hover to target the button). Finally I wouldn't add the border on hover as it doesn't really add anything in my opinion.

Also a very small nitpick: the copy button is currently slightly misaligned vertically, as already mentioned by Sam 🙈.
image

@danilo-leal
Copy link
Collaborator

Cool, thanks for the feedback! Having the copy button stay visible without hovering definitely makes sense 🤙

Don't worry about the alignment of the copy button, though ⎯ there's another PR that's tweaking its style and can also tweak its positioning to stay fixed! That's how it will look like:

Screen Shot 2023-07-03 at 12 27 03

@zanivan
Copy link
Collaborator

zanivan commented Jul 3, 2023

Just leaving my 2 cents here:

I believe that if the copy button stays, like the Danilo's last message shows, I see no need for the editable code, since the user will notice that the code is editable at the moment he hovers the container to copy. Besides, It'll add more visual complexity and therefore more cognitive load— also, it'll be one more hierarchy level if we take the new demo container being glued to the toolbar into account.

Moreover, I resonate with @alexfauquette: If we were to add more visual complexity to this component, maybe highlighting the expandable aspect of the code would be more worthy.

@mapache-salvaje
Copy link
Contributor

mapache-salvaje commented Jul 3, 2023

Great point about expanding code blocks—it feels like that's a different kind of action than, say, viewing the demo on CodeSandbox, which sits at the same level of interaction currently.

@danilo-leal
Copy link
Collaborator

Awesome, thanks y'all for the feedback! I think I'm leaning towards not heaving the header as the editable aspect can be communicated through other elements and thus avoiding having an additional element in the overall demo container hierarchy. The other points mentioned here (i.e. copy button positioning + promoting the expandability of the code block) will be touched on a different PR.

@gitstart really appreciate you for working on the PR up and collaborating! I'll be closing this one for one but definitely hoping to see you contributing more in the future 🤘

@danilo-leal danilo-leal closed this Jul 3, 2023
@oliviertassinari oliviertassinari added the scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305). label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[docs-infra] Copy button conflicting with the scrollbar

8 participants