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

[icons] Add X logo #38811

Merged
merged 9 commits into from
Dec 12, 2023
Merged

[icons] Add X logo #38811

merged 9 commits into from
Dec 12, 2023

Conversation

abreel
Copy link
Contributor

@abreel abreel commented Sep 5, 2023

Add X icon

import X from "@mui/icons-material/X"

<X />

Iterate on #38480

@mui-bot
Copy link

mui-bot commented Sep 5, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a4c2c1f

@zannager zannager added the package: icons Specific to @mui/icons label Sep 5, 2023
@zannager zannager requested a review from siriwatknp September 5, 2023 07:51
@agon21
Copy link

agon21 commented Sep 8, 2023

+1 on this PR. I need the updated icon from MUI

@Raspber
Copy link

Raspber commented Sep 9, 2023

so when can we see the update? i can provide the svg file if needed. I would like to use the new twitter icon with material without having to import my own icons into my project.

@ZeeshanTamboli
Copy link
Member

@siriwatknp Can you review this?

@davidslaby
Copy link

@siriwatknp could you review this PR, please? We need to use this icon in our app asap.
Attaching our support key: 74470

@samliu2022
Copy link

+1 on this PR. We need the updated icon from MUI

@justinmerrell
Copy link

Also bumping +1

@agon21
Copy link

agon21 commented Oct 24, 2023

@siriwatknp could you please review this? Thanks

@phenexfirf
Copy link

Bumping this as well. I would love to get this reviewed!

@jhenriod
Copy link

  • 1 - Any idea what is keeping this from passing some of the checks?

@fahmidme
Copy link

Bumping this

@siriwatknp siriwatknp changed the title Twitter icon update [icons] Update Twitter logo Nov 24, 2023
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Tested, it works! @michaldudak Can you do a final review?

@michaldudak
Copy link
Member

Since it's not called Twitter anymore, I'd be more inclined to create a new X icon and leave the old one as is (and remove it in the next major).

@sivangbagri
Copy link

please merge it fast , as a user I am also waiting

@umazhar
Copy link

umazhar commented Dec 3, 2023

Any updates on the icon?

@siriwatknp
Copy link
Member

siriwatknp commented Dec 6, 2023

Since it's not called Twitter anymore, I'd be more inclined to create a new X icon and leave the old one as is (and remove it in the next major).

🤔 But I think we should replace the "Twitter" icon because https://twitter.com/ logo is X. cc @oliviertassinari @mnajdova please help decide on this so that we can merge or continue.

@mbrookes
Copy link
Member

mbrookes commented Dec 12, 2023

because https://twitter.com/ logo is X

The service is called X, the logo is X, and the domain x.com (whoever owned that has probably since retired 😂) redirects to twitter.com, but presumably they'll flip that at some point (possibly afraid of the SEO impact?).

Name confusion aside, quite simply, changing the code of the Twitter icon component to display the X logo is a breaking change, and the component name no longer makes sense.

Pros:

  • Sites still using the old logo get the new one with no source code change needed. (mui.com home page anyone 🙈)

Cons:

  • It may unexpectedly break visual regressions tests.
  • Anyone specifically wanting to use the old Twitter icon (Admittedly an edge case) can't.

So, we simply add it as X.js, and keep Twitter.js (for now at least).

Why this wasn't done months ago is beyond me. This week please.

@siriwatknp
Copy link
Member

because https://twitter.com/ logo is X

The service is called X, the logo is X, and the domain x.com (whoever owned that has probably since retired 😂) redirects to twitter.com, but presumably they'll flip that at some point (possibly afraid of the SEO impact?).

Name confusion aside, quite simply, changing the code of the Twitter icon component to display the X logo is a breaking change, and the component name no longer makes sense.

Pros:

  • Sites still using the old logo get the new one with no source code change needed. (mui.com home page anyone 🙈)

Cons:

  • It may unexpectedly break visual regressions tests.
  • Anyone specifically wanting to use the old Twitter icon (Admittedly an edge case) can't.

So, we simply add it as X.js, and keep Twitter.js (for now at least).

Why this wasn't done months ago is beyond me. This week please.

Thanks for your comment. I'll create a new X logo.

@siriwatknp siriwatknp changed the title [icons] Update Twitter logo [icons] Add X logo Dec 12, 2023
@siriwatknp siriwatknp mentioned this pull request Dec 12, 2023
1 task
@brijeshb42 brijeshb42 merged commit d7eee3e into mui:master Dec 12, 2023
19 checks passed
@mbrookes
Copy link
Member

Thanks!

@oliviertassinari oliviertassinari added the new feature New feature or request label Dec 15, 2023
@kingeke
Copy link

kingeke commented Dec 29, 2023

Hey guys, great work so far, I can't seem to use XIcon or X on 5.15.2

Getting this error

SyntaxError: The requested module '/node_modules/.vite/deps/@mui_icons-material.js?v=db7be086' does not provide an export named 'X'

@michaldudak
Copy link
Member

michaldudak commented Dec 29, 2023

For some reason, the new icon wasn't added to the index file. Try importing it with import X from '@mui/icons-material/X' instead of import { X } from @mui/icons-material.

#40365 fixes this.

@kingeke
Copy link

kingeke commented Dec 29, 2023

@michaldudak thanks, worked.

@Edberaga
Copy link

Since it's not called Twitter anymore, I'd be more inclined to create a new X icon and leave the old one as is (and remove it in the next major).

Wait remove the old one? Please don't... :"(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: icons Specific to @mui/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.