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

Updating Twitter references and icons to Xitter [fixes #13980] #14041

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

reemhamz
Copy link
Contributor

@reemhamz reemhamz commented Jan 3, 2024

One-line summary

References to "Twitter" should be renamed to "X (formerly Twitter)", or just "X" where it makes sense, as well as updating Twitter icons to the new X(itter) icons.

Things to note

  • I'm not updating twitter.com links to x.com, because x.com links automatically reroute to twitter.com links... 🤦🏼
  • I won't be updating data-link-name attributes since those are internal and it would probably confuse/disrupt things from in our analytics

Note

I updated mentions of data-link-name to data-link-text because of the changes in this PR: #14057

Issue / Bugzilla link

#13980

Testing

@reemhamz
Copy link
Contributor Author

reemhamz commented Jan 9, 2024

Most references that need to be updated have been worked on.

Now I'm just waiting on the latest Protocol version to be published (#14084) so I can use the new X icon to replace the old Twitter bird icon

@reemhamz reemhamz changed the title Updating Twitter references and icons to Xitter Updating Twitter references and icons to Xitter [fixes #13980] Feb 7, 2024
@reemhamz reemhamz force-pushed the twitter-to-x branch 2 times, most recently from f30bec7 to 89764ce Compare February 20, 2024 02:04
@reemhamz reemhamz added Needs Review Awaiting code review Review: S Code review time: 30 mins to 1 hour labels Feb 20, 2024
@reemhamz reemhamz marked this pull request as ready for review February 20, 2024 02:29
Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

I took a look through the pages mostly everywhere seems to be covered (I think). I did notice a few pages that are still showing the old bird icon still.

The other thing I wonder is if we're perhaps overusing the aria-label attribute a bit too much here? From what I understand it's mostly meant as a fallback for when an interactive element does not contain an accessible name, but in all the places here the links already have accessible text (for icons, the text is only visually hidden)? I think we should probably only really be using aria-label as a last resort. Would it be better to use / update the already accessible link text instead?

l10n/en/brands.ftl Outdated Show resolved Hide resolved
l10n/en/firefox/nightly/whatsnew.ftl Outdated Show resolved Hide resolved
media/css/firefox/switch.scss Show resolved Hide resolved
@alexgibson alexgibson removed the Needs Review Awaiting code review label Feb 22, 2024
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.58%. Comparing base (7d67041) to head (b837b43).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14041      +/-   ##
==========================================
- Coverage   76.72%   76.58%   -0.15%     
==========================================
  Files         145      145              
  Lines        7868     7802      -66     
==========================================
- Hits         6037     5975      -62     
+ Misses       1831     1827       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

Updates look good! Spotted a couple of small things to tidy up but almost there!

One other small thing - there's a bird icon in the pocket mode footer still.

image

l10n/en/brands.ftl Outdated Show resolved Hide resolved
Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

r+ (I'll leave to you to merge when ready) - nice work!

@reemhamz reemhamz merged commit f264fd7 into mozilla:main Feb 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: S Code review time: 30 mins to 1 hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants