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] Add the Base UI logo with copy functionality #42446

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented May 29, 2024

Follow up to #42435 — This PR adds the Base UI logo to the navbars, together with the recently added copy SVG functionality. So, if you're visiting the Base UI docs, you should see the Base logo instead of the Material UI/MUI logo.

I generalized the recently created MuiLogoMenu component, changing its name to LogoWithCopyMenu. Then, the specific SVG component, together with the SVG strings of each logo, is passed via the _app.js file.

@danilo-leal danilo-leal added design This is about UI or UX design, please involve a designer scope: docs-infra Specific to the docs-infra product labels May 29, 2024
@danilo-leal danilo-leal self-assigned this May 29, 2024
@mui-bot
Copy link

mui-bot commented May 29, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against f1f1588

docs/src/icons/SvgBaseUiLogo.tsx Outdated Show resolved Hide resolved
docs/src/modules/components/AppFrame.js Outdated Show resolved Hide resolved
@danilo-leal danilo-leal requested a review from alexfauquette May 30, 2024 12:02
@michaldudak
Copy link
Member

michaldudak commented Jun 3, 2024

I created a PR in the Base UI repo that includes the logo: mui/base-ui#444. Seems to be working well (at least locally; there are some issues on CI, though).

@danilo-leal
Copy link
Contributor Author

@alexfauquette & @michaldudak can I ask y'all to give me a code-focused review? Let me know if there's anything that can be improved there 👍

const handleCopy = (svgSnippet: string) => {
setCopied(true);
copy(svgSnippet).then(() => {
setTimeout(() => setCopied(false), 3500);
Copy link
Member

Choose a reason for hiding this comment

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

What's the 3500 for?

Copy link
Member

Choose a reason for hiding this comment

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

It's the delay for the snack bar notifying that the element got copied.

image

Copy link
Member

@michaldudak michaldudak Jun 4, 2024

Choose a reason for hiding this comment

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

Could it be a constant (or at least have a comment in code)?
Besides, snackbars can be configured to auto-hide, so we don't need a setTimeout here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we need the setTimeout for the copy action, too, though? Like, similar to https://github.com/mui/material-ui/blob/next/docs/src/components/action/NpmCopyButton.tsx

docs/src/icons/SvgBaseUiLogo.tsx Show resolved Hide resolved
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks great

@danilo-leal danilo-leal merged commit 6ad092c into mui:next Jun 4, 2024
19 checks passed
@danilo-leal danilo-leal deleted the add-base-ui-logo-navbars branch June 4, 2024 18:24
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants