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] Update the Material logo + add copy functionality #42435

Merged

Conversation

danilo-leal
Copy link
Contributor

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

Updating the Material UI/MUI org logo with a fresher SVG (as per the last revision on December 2023) and creating the MuiLogoMenu component that allows to copy either just the logo icon or the full thing (logo + wordmark) via right click on it.

Right-click on the logo to see the copy menu:

SCR-20240528-umyz

Similar to Linear https://linear.app/

SCR-20240528-unrf

@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 28, 2024
@danilo-leal danilo-leal self-assigned this May 28, 2024
@mui-bot
Copy link

mui-bot commented May 28, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 12b163c

@@ -1323,6 +1323,23 @@ export function getThemedComponents(): ThemeOptions {
},
},
},
MuiSnackbar: {
Copy link
Member

@oliviertassinari oliviertassinari May 28, 2024

Choose a reason for hiding this comment

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

Off-topic

The more we use the brandingTheme, the more it feels like we should remove this pattern, and replace it with a wrapper of each Material UI component. The file starts to feel bloated. I almost wish Material UI had a CLI to automate this extraction.

Now, I guess that with Pigment CSS, moving from JS to CSS would help with performance, but I don't know. What the reality is. Maybe styled(styled()) is actually smaller than the current pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to discuss this further — I don't see this file as bloated; I like that all major customizations live within the theme file. The styled() approach would work, but the biggest downside I can think of is that I'd now need to open several different files instead of scrolling up and down on the same one, which sounds like a big DX impact.

Copy link
Member

Choose a reason for hiding this comment

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

I was focused on the UX part of the problem. For the DX, yeah, OK maybe.

Comment on lines +93 to +94
</MenuItem>
</Menu>
Copy link
Member

@oliviertassinari oliviertassinari May 28, 2024

Choose a reason for hiding this comment

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

I could clearly see a "Visit brand guideline" menu item that points to the right Notion page. I would use this more than the two other menu items 🙏. Today, I use the Notion search, but I always find that it's slow, so it would likely become my default go-to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I know. I'm just holding off for a bit as this is still, technically speaking, the organization logo and not the "Material UI" logo. I think it's better to hold off until we have the organization vs. flagship product brand sorted out.

Copy link
Member

Choose a reason for hiding this comment

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

I would imagine the branding assets will all be on one page: https://www.notion.so/mui-org/Branding-book-42011f0b6ad44e7ba4e507c00b2da13a?pvs=4, covering Material UI, Pigment CSS, etc so I don't see timeline considerations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm visiting the Material UI docs, I'd expect to have its brand assets on its docs space instead of somewhere else (where I'd see brand assets from other products). The timeline impact that I see is:

  • You're in mui.com, which is technically the org website. You right-click and see the "View brand guidelines" menu item that redirects you to that page.
  • On that page, we'd display the MUI logo with the "MUI" wordmark instead of "Material UI" because we haven't fully/firmly decided on a new company name to unlock the logo back to Material UI.
  • Given that the menu item is also accessible via the Material UI docs space, if you're redirected to a page that tells you that logo means MUI, that 200% cement that MUI = Material UI. I'm totally up for doing this, but I'd prefer doing it once it doesn't conflict with the organization's brand (whatever that is).

Copy link
Member

@oliviertassinari oliviertassinari May 29, 2024

Choose a reason for hiding this comment

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

Yeah OK. I guess I feel like a Notion page is good enough for this. It's simpler to execute, faster to keep up to date, and it matches the need for this, which feels rare. But no strong preferences. A dedicated page is undeniably more polished.

docs/src/components/action/MuiLogoMenu.tsx Outdated Show resolved Hide resolved
@colmtuite
Copy link
Contributor

colmtuite commented May 28, 2024

I figured the updated svg would be 32px rather than 30px, since 30px doesn't sit in a 4px or 8px scale. What's the thinking behind 30px? I think all of our UI elements should sit in a constrained spacing scale. 32px is pretty huge, I think the logo could even be 24px.

On the context menu, I'm not sure how I feel about blocking native behaviour. With this menu, we lose right-click menu options like "Open Link.." and "Copy Link Address". "Open Link in New Tab" is available via cmd+click but others are more difficult or impossible to access. On one hand, I appreciate that if someone right clicks the logo, they often want to copy the svg, so we're just making that easier. But on the other hand, those people have indicated that they know how to use dev tools to copy the svg. So we're blocking some people from doing what they wanted, to help other people who had other means to achieve what they wanted. Seems like a bad trade-off to me. But I've already talked way more about this than I originally intended to and despite this long message, I don't think it's a major deal 🤣 So I'm fine with whatever, but don't think it's a good idea to block native functionality like this.

@danilo-leal
Copy link
Contributor Author

32px is pretty huge, I think the logo could even be 24px.

The revised SVG in itself is 24 x 24 (and it's the size you get by copying it—see the viewBox attribute); I'm just using it in a different size on the navbars based purely on visual balance: 24px looks way too small in the navbars, I think. (And looking at it side-by-side like this, it could even be a tiny bit smaller, like 28px).

24x24 30x30
Screenshot 2024-05-28 at 19 14 11 Screenshot 2024-05-28 at 19 14 47

So we're blocking some people from doing what they wanted, to help other people who had other means to achieve what they wanted.

Yeah, I hear the overall concern! There are other sites that put some of the native options (like Resend) on the custom menu, and it's something we could as you do lose those options, but I feel like the right-click to copy the logo is becoming common even for folks who wouldn't know how to open the dev tools to copy the SVG (which is why I quoted this specific sentence). So, maybe it is worth it? It's also faster to copy the logo this way, even for us, than using the dev tools. How many times have you used the native menu on the logo link specifically?

@colmtuite
Copy link
Contributor

I feel like the right-click to copy the logo is becoming common even for folks who wouldn't know how to open the dev tools to copy the SVG

This part confused me. How would someone intend to copy the logo by right-clicking if they didn't know how to open dev tools? I would have guessed that anyone who is right-clicking the logo is intending to either open dev tools (and we're helping these people) or trying to open the link in a new tab/window (we're blocking these people and they have no other way to achieve what they wanted to do, besides copying the url from the address bar, opening a new tab, and pasting the url)?

How many times have you used the native menu on the logo link specifically?

I never use the native menu on logos. I use cmd+alt+C to open dev tools, and use cmd+click to open links in new tabs. So this feature wouldn't help or hinder me personally. I'm mostly concerned with less tech savvy people who always right-click to open links in a new tab.

@danilo-leal
Copy link
Contributor Author

How would someone intend to copy the logo by right-clicking if they didn't know how to open dev tools?

What I was getting at, but expressed poorly, was that the pattern of "right-clicking a logo to open a custom menu that allows copying its SVG" is getting popular enough to the point that it isn't a big disruption, maybe even expected, and also more convenient than the dev tools... particularly more so for folks who are less tech-savvy and wouldn't know how to get to the SVG node to copy it.

@colmtuite
Copy link
Contributor

Ah ok, gotcha. I wasn't aware that it's a common pattern, didn't even know about Linear or Resend. So yeah I can see arguments on both sides. Approved anyway ✅

@noraleonte
Copy link
Contributor

This looks great 👌 🎉
But I feel like the highlighting is a bit confusing 🤔 The menu seems to have an item selected by default, but when you hover on the other item, they both look the same 🙈 Was that intentional?
I'm wondering if having one of these selected by default is even necessary 🤔 In my mind, it should be a simple click action: I click on one of these, something happens. Having an item pre-selected and having to click again for something to happen is a bit counterintuitive

Recording.2024-05-29.125745.mp4

@danilo-leal
Copy link
Contributor Author

danilo-leal commented May 29, 2024

@noraleonte Yeah, that was something I should've highlighted in a comment, but I didn't intend to have one item focused by default! Couldn't quite figure out yet what's causing this behavior, though 🤔 Any ideas?
Update: that's fixed with the last commit!

Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

@danilo-leal Sorry for the late reply, I missed the notification 🙈
Looks great 🎉

@colmtuite
Copy link
Contributor

@danilo-leal @noraleonte This is a known issue with the Material UI menu components where hovering an item doesn't steal focus. The menus currently have highlighted, focus, and hover states. They should just have a focus state. When you hover a menu item, it should steal focus.

We'll change this in the upcoming Base UI menu components.

@noraleonte
Copy link
Contributor

@colmtuite Thanks for clarifying 🎉 I thins case I was mostly referring to the styling of these states. IMO there should be a clear visual distinction between hover/focus/selected states as a general rule.

@danilo-leal danilo-leal merged commit 7b3a164 into mui:next May 29, 2024
22 checks passed
@danilo-leal danilo-leal deleted the update-material-logo-and-add-copy-func branch May 29, 2024 16:00
@oliviertassinari
Copy link
Member

oliviertassinari commented May 29, 2024

Material UI menu components where hovering an item doesn't steal focus

@colmtuite I guess this depends on components. For the combobox, there is a push in the opposite direction: #23916.

@colmtuite
Copy link
Contributor

colmtuite commented May 29, 2024

Not sure, will need to investigate when we get to Combobox. Combobox (as implemented on google.com) may be different, because moving focus via keyboard actually changes the value of the input, whereas hovering a different item doesn't change the value. That's not the case with Select or DropdownMenu, so it's possible they should be treated differently, not sure yet.

It seems there are a few issues being conflated in that thread you linked. When you move the cursor outside of a menu/select, the most recently highlighted item should remain highlighted. But that's because it's focused, and focus persists even when you move your cursor outside the menu. If the highlight is removed it would be misleading. Imagine if menu item focus states were typically styled by using a focus ring rather than changing the background-color. If that were the case, I imagine less people would be confused by it.

@oliviertassinari
Copy link
Member

It seems there are a few issues being conflated in that thread you linked

True

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.

None yet

6 participants