[docs-infra] Limit the copy button to the visible code block

danilo-leal commented 2 weeks ago

[x] I have followed (at least) the PR section of the contributing guide.

Old description

This PR is a follow-up to https://github.com/mui/material-ui/pull/41827. What motivated this one is a reflection around multi-tab demos where I initially thought that the demo toolbar's copy button copying the content from both tabs was a valuable thing to support. However, in discussing it with @colmtuite & @zanivan, I realized this is actually a non-intuitive UX. Here's why that is: 1. The PR I linked above introduced somewhat of an inconsistency: if there's a demo toolbar, the copy button will appear on it. If there's not, the copy button is inside the code block. A better approach for managing the user's expectations is for this button to always be in the same place. 2. It seems way more intuitive to have the copy button closer to the content it will copy (i.e., the code block). 3. In the case of multi-tab demos, there's no affordance for users that lets them know the copy button on the toolbar will copy content from both tabs; this can get them surprised and create frustration, because we expect that, more often than not, you want to copy _only_ what you're seeing, and not what it's hidden in a separate tab. On a more logistical note, I know we already have the code block copy button wired in GA, but I'm not sure if we want to do anything differently here if we're removing the button and its respective `eventAction` from the code. I considered swapping the `eventAction` from the remaining button to the toolbar's one, but I'm not sure — y'all let me know.

Edit: This PR has gone through quite the discussion, so this is the updated summary of what it is doing:

Changes the plain code block copy button from icon-based to word-based
When in multi-tab demo containers, the demo toolbar copy button just copies the active tab content, as opposed to concatenating and copying content from the two tabs

Preview:

mui-bot commented 2 weeks ago

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad) No bundle size changes

Generated by :no_entry_sign: dangerJS against d68506cf2b06361f2422cc1896ee2ea8978a1d52

oliviertassinari commented 2 weeks ago

I was expecting that we remove the opposite copy button. I don't use this one (it's hard to see):

but I use this one (especially for multi-tab demos to not have to stitch tabs together):

like most users according to Google Analytics https://github.com/mui/material-ui/pull/41827#issuecomment-2046026830.

danilo-leal commented 2 weeks ago

That is what is currently live in next, that I pushed in https://github.com/mui/material-ui/pull/41827. However, in this PR, the thought process is (as I tried to cover in the description):

the one within the toolbar is more distant from the code, which could be less intuitive. The GA events might illustrate the opposite, which is interesting, but it could be that the copy button within the code block is a bit too opaque (as you mentioned, it can be hard to see), and improving that would help boost its usage
particularly in the case of multi-tab demos, the toolbar one is confusing because it currently copies content from both tabs. Being in the toolbar doesn't immediately communicate it will copy from both tabs, but maybe it does more than being in the code block. However, we're parting from the thinking that copying from both tabs is, in the end, a non-desired UX because you'd be more likely to want to copy what you're seeing rather than the full content spanning multiple tabs. Additionally, what will frequently stored in the second tab is data that will likely be replaced by contextual data from the developer's project (e.g., Autocomplete data set)

Does that make sense?

oliviertassinari commented 2 weeks ago

@danilo-leal Ok, got it. I guess It's that I'm mostly blind to the problems with the current version of HEAD. I see the missing copy button for each tab, but I don't see the other problems.

colmtuite commented 1 week ago

@danilo-leal @oliviertassinari Hmmm this GA data is over a massive sample size. It seems like we should consider this more. We should try to figure out why people are not using the code block button. Perhaps first make it more prominent without remove the other button?

danilo-leal commented 1 week ago

Yeah, well, there's a world where, as Olivier said, this is a non-issue. Or maybe the only substantial issue here is when it's a multi-tab demo, and the copy button secretly copies content from the two tabs? If we just changed that so it copied the content from only the visible tab, that could be progress (although the layout feels a bit weird, still).

zanivan commented 1 week ago

If we just changed that so it copied the content from only the visible tab, that could be progress (although the layout feels a bit weird, still).

I think it works, given that users seems to prefer the toolbar button, and it'll suit the UX for copying only what you can see.

We can keep monitoring feedbacks about it, and review this decision if users aren't finding the copy button or if they want a solution to copy the code from both code blocks at the same time.

danilo-leal commented 1 week ago

We haven't yet used the multi-tab demo feature extensively, so if we grew that first, I'd expect to see more feedback pouring in. That could also be a way to go before what we have going on here — I'd be happy to temporarily close this PR, expand that feature's usage, and wait to see what folks think.

colmtuite commented 1 week ago

maybe the only substantial issue here is when it's a multi-tab demo, and the copy button secretly copies content from the two tabs?

This was my main concern, I'm concerned people are mostly using this button specifically because it copies code from both tabs? If that's not the reason they use this button, then I guess it doesn't really matter. It's probably just a matter of visual prominence in that case.

danilo-leal commented 1 week ago

If that's not the reason they use this button

I'd say it isn't because very few demos (if any, honestly!) currently use the multi-tab demo feature, so they mostly use the toolbar button to copy the code block content. I don't think we have had feedback yet about whether copying content from both tabs through the button they'd usually use to copy the code content is intuitive/good or not. So, I guess maybe the course of action here is to really do nothing, use the multi-tab demo feature more, and wait for feedback? I'd expect some folks to point out that they'd like to copy just what they see, but I'm willing to be proven wrong! Does that resonate?

colmtuite commented 1 week ago

I'd expect some folks to point out that they'd like to copy just what they see

Yeah this was my expectation too. If it's recently implemented behaviour and can't account for much of the GA data, then I still feel confident about this.

I think it's weird/bloated/confusing to have two copy buttons, so I'd be in favour of trying something. Maybe a good path forward is to try to get more data by making the code block button more prominent. Perhaps changing from an icon to "Copy" and increasing the opacity. Then reassess the data in a month or so.

danilo-leal commented 1 week ago

Just to recap a bit.

The current state of the docs looks generally fine to me. We don't have two buttons going on anymore — in the case of a demo container, the only copy button is the one from the toolbar, whereas, in a code block case, the copy button is within the block.

Intentionally said "anymore" because that was the reality a few weeks ago before https://github.com/mui/material-ui/pull/41827. And even then, the GA data showed that on demo containers that had the two buttons, folks were clicking more on the one from the toolbar.

The issue trigger is really the multi-tab demos. It seems we're all on the same page that copying content from both tabs is not great, and the solution to that in terms of affordance would be what this PR does: it moves the button to inside the block, so it's clearer what will be copied is that particular code content. However, folks seem to be pretty used to clicking the button on the toolbar, and this positioning change is a risk. We could move on with this PR's changes and counter the risk by making the button within the block more visible, but we could also do nothing, scrap the PR, extend the use of the multi-tab demos, and see if folks will agree with us that copying content from both tabs is bad. If they do, we can come back here and push forward. If they don't, the button on the toolbar for that use case sort of makes sense (i.e., seems more "global" than within the block)?!

Long-winded answer just to make sure we're aligned 😅

colmtuite commented 1 week ago

Ok gotcha, thanks for the context. I think I understand the backstory/issue better now.

First, a couple of assumptions to qualify my suggestion below:

see if folks will agree with us that copying content from both tabs is bad

It seems clear to me that this is a surprising/confusing/undesirable UX. But who knows, I'm just guessing. I just can't imagine how you'd expect that behaviour, or why you'd want it.

the GA data showed that folks were clicking more on the one from the toolbar

I'm guessing that the reason most people are clicking the toolbar button is because the one inside the code block is difficult to see. Olivier mentioned this visibility issue too.

So here are my current thoughts based on the above assumptions:

I think having the copy button in different places at different times is confusing.
I think having a 50% opacity icon button is problematic.
I think having the button copy inactive tabs is unexpected/undesirable.

And here are my current thoughts in a bit more depth:

I think placing the copy button in the toolbar creates an issue, because we have some code blocks that don't have a toolbar and we don't want to have two copy buttons in two different places for the same demo. So that creates an issue where sometimes the copy button is in the toolbar, and other times it's in the code block. I think that's confusing/disorienting. I think we should have just one place where the copy button always exists. That way it's consistent, and people can learn where it is.

Since we need to have code blocks that don't have a toolbar, the only way to achieve this consistent button location would be to always have the copy button inside the toolbar, on all demos and code blocks.

So my suggestion is this:

Remove the button from the toolbar.
Always place the button in the code block
Change it from an icon to "Copy", and increase the opacity.
Have it copy the active file/tab only.

Alternatively:

For demos, leave the button in the toolbar, with no button in the code block.
Where there is a code block with no toolbar, have the button in the code block, but change it from an icon to "Copy", and increase its opacity.
Where there are multiple tabs, have the toolbar button only copy the active tab.

This solution only works if there is never a case where we have a multi-tab code block that has no toolbar. It also suffers from the confusing conditional button location issue. But I feel like this solution is not too bad.

zanivan commented 1 week ago

I agree with @colmtuite's reasoning. I’d like to add that if we proceed with the proposed changes—removing the toolbar button and improving the code block button—we can still monitor usage as a health metric and keep an eye on the docs-feedback channel for any issues that arise. If we notice a decrease in copy events, we can iterate on it, similar to what we did with the API table layout.

colmtuite commented 1 week ago

I edited my previous comment.

danilo-leal commented 1 week ago

I'm on board with the suggestions and the reasoning — we were on the same page already! 😬 The above-pushed commit experiments with refining the copy button design towards suggestion one.

One thing that could make the first iteration of this PR (and which is close to suggestion 2) more appealing is that we have instances where the demo container renders with the code block hidden, requiring a click on the "Show code" button to show it. Given that the copy button would live inside the code block in this iteration, visitors would need to expand the demo to get the copy button, whereas today, even if it's closed, they can click copy on the toolbar and get the code. All in all, though, not sure if it's a strong enough reason to give up on this path.

bharatkashyap commented 1 week ago

Adding some slight context on the "why" behind having the Toolbar Copy button copying content from multiple tabs:

In most scenarios, people who copy content from demos intent to paste it into files and expect to see those changes work for them instantly; the idea with the Toolbar copy button (as being separate from the "local" copy button inside the editor) was that it would allow us to have this behaviour of a single copy action allowing a developer to copy everything they need for the demo to work when pasted in their local editor.

colmtuite commented 1 week ago

expect to see those changes work for them instantly

@bharatkashyap I'm confused how that work though, because is it not the case that the files/tabs need to be pasted into different local files? For Base UI, we will use the multi-tabs feature for a JSX file and a plain CSS file. You can't paste both these tabs into the same local file...it will throw an error.

We will also use the multi-tabs feature for Tailwind examples. We will have a JSX tab and a TW config tab. Again, you don't want to paste these tabs into the same local file.

Given that the copy button would live inside the code block in this iteration, visitors would need to expand the demo to get the copy button, whereas today, even if it's closed, they can click copy on the toolbar and get the code.

@danilo-leal This is a good point I overlooked. This makes me lean towards the "Alternatively" part of my suggestion above.

bharatkashyap commented 1 week ago

expect to see those changes work for them instantly

@bharatkashyap I'm confused how that work though, because is it not the case that the files/tabs need to be pasted into different local files? For Base UI, we will use the multi-tabs feature for a JSX file and a plain CSS file. You can't paste both these tabs into the same local file...it will throw an error.

We will also use the multi-tabs feature for Tailwind examples. We will have a JSX tab and a TW config tab. Again, you don't want to paste these tabs into the same local file.

In certain use cases – for instance when a large number of utility functions and some datasets are placed in multiple tabs – copying the content from a single tab would result in the pasted content not compiling since a few referenced variables would be undefined. However, I agree with you that this use case might be less common in the future compared to having .css files or Tailwind styles defined in separate tabs, where copying all content together would not make sense.

I agree with your proposal in the "Alternatively" section save for point 3., where I think the copy button can attempt to be smarter and copy content from either all tabs when there are only .js/.jsx files in the demo, or only the active tab when there are .css files involved.

colmtuite commented 1 week ago

Thanks @bharatkashyap. Sounds like the "alternatively" section above makes sense. I don't have a strong opinion on the smart/conditional button approach.

danilo-leal commented 1 week ago

Thanks for chiming in, @bharatkashyap! 🙏

I was about to suggest that we could have some sort of config where, instead of assuming from the file extension, we could manually select whether the toolbar button copies the content from all tabs vs. copying just from the visible tab (which seems to be the Data Grid/Autocomplete vs. Base UI demo examples, respectively) but it sounds too complex, and ultimately, for the user, non-predictable (times does X, times does Y). It should be better to lean towards a heuristic here and go for it — I'd choose to copy just the content visible. As I described in the PR's description, I'd expect folks to replace dummy Autocomplete/Data Grid data with their own data anyway, so it seems fine if the copied content doesn't run immediately (and they can always copy manually from the other tab, too, no biggie).

Seems like we have a way to go, then! I'll push the rest of the changes in a bit and request another review!

danilo-leal commented 1 week ago

Okay, can y'all double-check if what I'm pushing on this last commit makes sense (ergonomic, organized, functional, etc.)? Here's a rundown:

I figured that in order for the toolbar copy button to copy the content just from the active tab, it needed to have access to which tab is active, which the DemoToolbar component doesn't. So, I added props so we could handle that on the Demo component instead.
with that, I then placed the handleCopyClick function in the Demo component, which accesses the active tab and determines which content the copy button should grab. Also, I thought it'd be nice/consistent to change the icon (which we weren't doing), so I'm also treating that in the Demo component via the newly added copyIcon prop.

On my end here, everything seems to be working as expected. You can check the changes here after the deploy preview is done. Try copying content from the first demo and then the last (which uses the multi-tab).

danilo-leal commented 1 week ago

Here's what I see, just for double-confirmation (and I'll also update the PR's title & description to match the latest state of the discussion):

https://github.com/mui/material-ui/assets/67129314/6dccecd2-2943-4114-8015-77c71073840a

mui / material-ui

[docs-infra] Limit the copy button to the visible code block #42115

Netlify deploy preview

Bundle size report