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

Share page component updates to new "𝕏" icon #2883

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

adi-unni
Copy link
Contributor

@adi-unni adi-unni commented Nov 2, 2023

What is the context of this PR?

Old component had the old twitter bird icon. New component has the new SVG for "𝕏" icon. Additional change has been made to fix alignment issues between the different icons. Now all icons are in line

How to review this PR

Go to share page, and check icons are in line, and the new twitter "𝕏" icon can be seen.

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

@adi-unni adi-unni added the Bug Something isn't working label Nov 2, 2023
@adi-unni adi-unni self-assigned this Nov 2, 2023
@adi-unni adi-unni linked an issue Nov 2, 2023 that may be closed by this pull request
Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit 1cf0c7a
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6548f69840bb9000084c67ec
😎 Deploy Preview https://deploy-preview-2883--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Other than the SVG stuff looks good. Only question I have is do we change all references to "Twitter" to "X"?

Also just a reminder we don't need to add PRs to the DS project board anymore, just because the issue card is already there.

src/components/icon/_macro.njk Outdated Show resolved Hide resolved
@adi-unni
Copy link
Contributor Author

adi-unni commented Nov 6, 2023

Team has been consulted. Whilst twitter documentation is unclear and their marketing material is even more (the main page is still www.twitter.com but the page title is X and the logo is X), the decision has been made that the hyperlink text should be kept as twitter.

@adi-unni adi-unni merged commit 81c18ec into main Nov 7, 2023
8 checks passed
@adi-unni adi-unni deleted the fix/2867/share-page-component-needs-updating branch November 7, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Share page component needs updating
3 participants