-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[docs-infra] Limit the copy button to the visible code block #42115
Conversation
Netlify deploy previewhttps://deploy-preview-42115--material-ui.netlify.app/ Bundle size report |
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): |
That is what is currently live in next, that I pushed in #41827. However, in this PR, the thought process is (as I tried to cover in the description):
Does that make sense? |
@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. |
@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? |
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). |
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. |
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. |
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. |
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? |
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. |
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 #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 😅 |
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:
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.
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:
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:
Alternatively:
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. |
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. |
I edited my previous comment. |
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. |
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. |
@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.
@danilo-leal This is a good point I overlooked. This makes me lean towards the "Alternatively" part of my suggestion above. |
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. |
Thanks @bharatkashyap. Sounds like the "alternatively" section above makes sense. I don't have a strong opinion on the smart/conditional button approach. |
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! |
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:
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing a demo with multiple tabs. The last demo on the linked page has a JS tab and a disabled TS tab, for me at least.
But everything else looks good. So assuming the copy button only copies the active tab, then 🚢
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): Screen.Recording.2024-05-08.at.14.15.23.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to me! Everything seems to be working as intended :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I updated the title such that it reflects the modification 👍
@@ -741,6 +707,8 @@ export default function DemoToolbar(props) { | |||
DemoToolbar.propTypes = { | |||
codeOpen: PropTypes.bool.isRequired, | |||
codeVariant: PropTypes.string.isRequired, | |||
copyButtonOnClick: PropTypes.object.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong type fixed in #41994 (comment)
Old description
This PR is a follow-up to https://github.com//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: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 theeventAction
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:
Preview: