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

[docs-infra] Fix GoogleAnalytics missing event for code copy #38469

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

alexfauquette
Copy link
Member

Related to #38421

@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Aug 14, 2023
@mui-bot
Copy link

mui-bot commented Aug 14, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4fecd05

@@ -22,6 +23,7 @@ export default function CodeCopyButton(props: CodeCopyButtonProps) {
className="MuiCode-copy"
onClick={async (event) => {
event.stopPropagation();
Copy link
Member

@oliviertassinari oliviertassinari Aug 14, 2023

Choose a reason for hiding this comment

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

Very strange: Why do we stop propagation?

👍 to remove it, it seems to be fine without.

Copy link
Member Author

Choose a reason for hiding this comment

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

No Idea, it's been here since the creation of editable code in #34454

@bharatkashyap do you remember why you introduced it?

Copy link
Member

@oliviertassinari oliviertassinari Aug 14, 2023

Choose a reason for hiding this comment

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

In the unknown, it could be nice to drop it, for the sake of learning faster. If it's really needed, we can add a comment. I don't see why it would help. stopPropagation() are usually flags, most are harmful: https://css-tricks.com/dangers-stopping-event-propagation/.

Copy link
Member

@bharatkashyap bharatkashyap Aug 16, 2023

Choose a reason for hiding this comment

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

I don't remember specifically adding it, 👍 to remove it.

I'll try and dig deep to find where it came from separately.

Edit: I think it came from 9017db6

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Aug 14, 2023
onClick={async (event) => {
event.stopPropagation();
onClick={async () => {
// event.stopPropagation();
Copy link
Member Author

Choose a reason for hiding this comment

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

I commented such that if someone investigates a bug related to it they can get the blam easily

@alexfauquette alexfauquette merged commit 7c6de87 into mui:master Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants