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

Social Links: add X #54092

Merged
merged 10 commits into from
Sep 25, 2023
Merged

Social Links: add X #54092

merged 10 commits into from
Sep 25, 2023

Conversation

jeherve
Copy link
Contributor

@jeherve jeherve commented Aug 31, 2023

Fixes #53223

What?

This adds a new Social Link icon, for the X service (aka Twitter).

Why?

X remains a popular service, and some folks may want to use the new service name and icon on their site.

How?

It doesn't replace the existing Twitter icon, as discussed in the issue above. Instead, it adds a new icon that can be found when searching for X or for Twitter.

Testing Instructions

  1. Go to Appearance > Site Editor
  2. Insert a Social Icons block
  3. In that block, search for "X" or "Twitter", and try the new icon that pops up.

Testing Instructions for Keyboard

This PR doesn't change keyboard interactions.

Screenshots or screencast

Screenshot 2023-08-31 at 14 32 22 Screenshot 2023-08-31 at 13 37 03 image

This will allow people to find the new icon when searching for Twitter.

See WordPress#53223 (comment)
@carlomanf
Copy link

Screenshot 2023-08-31 at 14 32 22

The old one still shows in the search results. It should be hidden, or users might think they need to add both.

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the PR, @jeherve!

I think the code is mostly fine, but I have two points I would like to discuss.

  • The question is whether the slug should be x. For example, if we use x-twitter, twitter-x, etc., the keyword itself in the block variation will be unnecessary. Incidentally, x-twitter is used in Font Awesome.
  • I would like to ping @WordPress/gutenberg-design to confirm the design of the SVG icon 🙏

packages/block-library/src/social-link/index.php Outdated Show resolved Hide resolved
packages/block-library/src/social-link/variations.js Outdated Show resolved Hide resolved
@jameskoster
Copy link
Contributor

Icon looks fine to me.

Not entirely sure I understand the reasoning behind keeping the old block / icon... Anyone using the Twitter block will need to manually switch over which seems a bit of a pain. Perhaps there's a reason that someone would want to use the old logo that I'm not seeing?

jeherve and others added 2 commits September 4, 2023 13:30
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
@jeherve
Copy link
Contributor Author

jeherve commented Sep 4, 2023

Thanks for the feedback. I've pushed the changes you suggested.

  • The question is whether the slug should be x. For example, if we use x-twitter, twitter-x, etc., the keyword itself in the block variation will be unnecessary. Incidentally, x-twitter is used in Font Awesome.

We definitely can, although it seems that would be a first with social links here. I'll let someone else decide if that's okay. Just let me know!

Not entirely sure I understand the reasoning behind keeping the old block / icon... Anyone using the Twitter block will need to manually switch over which seems a bit of a pain. Perhaps there's a reason that someone would want to use the old logo that I'm not seeing?

This was discussed here:
#53223 (comment)

I think this was suggested because some folks may not want to use the new name and logo on their sites.

@carlomanf
Copy link

Not entirely sure I understand the reasoning behind keeping the old block / icon... Anyone using the Twitter block will need to manually switch over which seems a bit of a pain. Perhaps there's a reason that someone would want to use the old logo that I'm not seeing?

This was discussed here: #53223 (comment)

I think this was suggested because some folks may not want to use the new name and logo on their sites.

The comment you linked to doesn't say anything about what users want to use, it just suggests keeping the old one without giving a reason. I also don't see why someone would object to merely the logo but happily continue to use the service and link to their profile, wouldn't they want to remove the link altogether?

@jeherve
Copy link
Contributor Author

jeherve commented Sep 4, 2023

The comment you linked to doesn't say anything about what users want to use, it just suggests keeping the old one without giving a reason.

I think the key in that comment was this part:

less forceful of a change on our end

I think there can be a few examples where this makes sense:

  • I could imagine some folks added the Twitter logo to their site last year, and who haven't made a lot of changes to their site since then. They may not be aware of the recent changes in Twitter's branding and editorial direction, and may be confused if their site starts showing a "X" black symbol from one day to the next.
  • They may be aware of the changes, but may want to keep the blue bird icon since that's a logo their visitors may easily recognize.
  • They may know about the changes, and may not wish to display the new logo on their site. They may prefer to show that they preferred "old Twitter", for example.

That said, automatically updating everyone to the new logo may make sense as well, I personally do not have any strong opinion for or against.

I also don't see why someone would object to merely the logo but happily continue to use the service and link to their profile, wouldn't they want to remove the link altogether?

That's a good point. If you do not like the new logo or the changes that happened with Twitter in the past few months, maybe you'd be better off removing the link entirely, instead of keeping either a Twitter logo or an X logo. We cannot make that choice for the site owner though.

@carlomanf
Copy link

  • They may know about the changes, and may not wish to display the new logo on their site. They may prefer to show that they preferred "old Twitter", for example.

I have seen some profiles start using the old Twitter logo as their profile picture, which seems to convey the meaning, but in the social links context, I don't think the intent is clear and it just looks like they are late to update it. I also think this is an extremely small number of users.

  • I could imagine some folks added the Twitter logo to their site last year, and who haven't made a lot of changes to their site since then. They may not be aware of the recent changes in Twitter's branding and editorial direction, and may be confused if their site starts showing a "X" black symbol from one day to the next.

While the confusion is plausible, they would figure it out within minutes, so this isn't really a scenario of someone consciously wanting to keep the old logo.

  • They may be aware of the changes, but may want to keep the blue bird icon since that's a logo their visitors may easily recognize.

I don't think this is a scenario of someone wanting to keep the old logo permanently, so this might just be an argument to delay the change?

@ryanwelcher ryanwelcher added [Type] Feature New feature to highlight in changelogs. [Package] Block library /packages/block-library labels Sep 5, 2023
@richtabor
Copy link
Member

I think it’s fine to add the X icon alongside the Twitter icon.

I don’t love changing people’s sites right off; but rather give them the choice to use the new icon if they’d like to.

@fabiankaegy fabiankaegy added the [Block] Social Affects the Social Block - used to display Social Media accounts label Sep 6, 2023
@t-hamano
Copy link
Contributor

t-hamano commented Sep 10, 2023

Discussions at FontAwesome may be helpful in determining whether to replace existing icons or add new ones; at FontAwesome, it appears that they were eventually added as new icons.

If we add a new one in Gutenberg as well, is it OK to leave the slug as x? Or x-twitter or twitter-x?

@jameskoster
Copy link
Contributor

I don’t love changing people’s sites right off; but rather give them the choice to use the new icon if they’d like to.

I do agree but the problem is; if you maintain dozens of sites, each with multiple instances of the Twitter block, and you just want to use the correct branding then it's going to be a real pain updating them all.

An idea:

  1. Add X as a new block for 6.4.
  2. Replace Twitter with X for 6.5 and release a "Twitter" block plugin.

This way folks have a way to use the new branding asap, albeit manually. Over time instances of the Twitter block get updated, and folks who need to use the old logo can install a plugin to do so.

@richtabor
Copy link
Member

Add X as a new block for 6.4.
Replace Twitter with X for 6.5 and release a "Twitter" block plugin.

Yea, that's probably fine.

@t-hamano
Copy link
Contributor

I noticed a difference in line thickness between the logo published on the brand toolkit page and the logo actually used in the application. I cannot determine which was intended, but this may be addressed later.

Desktop App (thick line)

desktop

iPhone App (thick line)

mobile

Brand Toolkit Page (thin line)

brand-toolkit

Copy link
Member

@ndiego ndiego left a comment

Choose a reason for hiding this comment

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

This is looking really good, thanks for working on this! Just a small tweak to the logo and a label change, then I think this is good to go. 💪

jeherve and others added 2 commits September 25, 2023 09:56
Copy link
Member

@ndiego ndiego left a comment

Choose a reason for hiding this comment

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

Works like a charm. LGTM! 💪

@mikachan mikachan added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 25, 2023
@priethor priethor merged commit 5da68dc into WordPress:trunk Sep 25, 2023
49 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 25, 2023
mikachan pushed a commit that referenced this pull request Sep 26, 2023
* Social Links: add X

Fixes #53223

* Add Twitter keyword to variation

This will allow people to find the new icon when searching for Twitter.

See #53223 (comment)

* Reorder links alphabetically

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* No need for a capital letter

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* Fix svg attributes

See #54092 (comment)

Co-authored-by: Rich Tabor <hi@richtabor.com>

* Remove "icon"

Co-authored-by: Nick Diego <nickmdiego@gmail.com>

* Update X icon path

See #54092 (comment)
See #54092 (comment)

---------

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Rich Tabor <hi@richtabor.com>
Co-authored-by: Nick Diego <nickmdiego@gmail.com>
@mikachan
Copy link
Member

I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 0deec5f

@mikachan mikachan removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 26, 2023
mikachan pushed a commit that referenced this pull request Sep 26, 2023
* Social Links: add X

Fixes #53223

* Add Twitter keyword to variation

This will allow people to find the new icon when searching for Twitter.

See #53223 (comment)

* Reorder links alphabetically

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* No need for a capital letter

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* Fix svg attributes

See #54092 (comment)

Co-authored-by: Rich Tabor <hi@richtabor.com>

* Remove "icon"

Co-authored-by: Nick Diego <nickmdiego@gmail.com>

* Update X icon path

See #54092 (comment)
See #54092 (comment)

---------

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Rich Tabor <hi@richtabor.com>
Co-authored-by: Nick Diego <nickmdiego@gmail.com>
Mamaduka added a commit that referenced this pull request Sep 26, 2023
* use the wporg cdn (#54795)

* core-data: Fix nested property access with undefined name (#54790)

* core-data: Fix nested property access with undefined name

* Add a unit test

* Social Links: add X (#54092)

* Social Links: add X

Fixes #53223

* Add Twitter keyword to variation

This will allow people to find the new icon when searching for Twitter.

See #53223 (comment)

* Reorder links alphabetically

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* No need for a capital letter

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

* Fix svg attributes

See #54092 (comment)

Co-authored-by: Rich Tabor <hi@richtabor.com>

* Remove "icon"

Co-authored-by: Nick Diego <nickmdiego@gmail.com>

* Update X icon path

See #54092 (comment)
See #54092 (comment)

---------

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Rich Tabor <hi@richtabor.com>
Co-authored-by: Nick Diego <nickmdiego@gmail.com>

* remove font files created by tests after tests run (#54771)

* Check that new pattern is synced before replacing existing blocks with a synced pattern (#54804)

* Patterns: Improve sentence case consistency of labels and notices (#54807)

* Navigation block: fix padding on mobile overlay when global padding is 0 (#53725)

* force min value for padding to be 2rem

* fallback for when the css variables are not defined

* Allow the padding to be smaller than 2rem

* Add fix to avoid trigger hover state on links when overlay opens

---------

Co-authored-by: scruffian <ben@scruffian.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Always show the total number of patterns even with only one page (#54813)

* Always show the total number of patterns even with only one page

* Add to explorer too

* Hide total number of 0

* Font Library: Avoid rendering Font Library UI outside Gutenberg plugin (#54830)

* Remove action to fix tests. (#54806)

* Conditionally remove deprecated 'print_emoji_styles' (#54828)

* Fix Performance tests

* Fix global styles revision

---------

Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Rich Tabor <hi@richtabor.com>
Co-authored-by: Nick Diego <nickmdiego@gmail.com>
Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Maggie <maggie.cabrera@automattic.com>
Co-authored-by: scruffian <ben@scruffian.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts Needs User Documentation Needs new user documentation [Package] Block library /packages/block-library [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social Links Block: update Twitter Logo