-
Notifications
You must be signed in to change notification settings - Fork 50
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
Update Twitter to X branding #567
base: develop
Are you sure you want to change the base?
Conversation
ChanceM
commented
Sep 11, 2023
Maybe a white X is more readable on a dark background? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great. But I will say that this PR should include updating the Twitter logo from the show pages as well. That should be as easy as adding another else if to the links.html file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something but I do not see where this new partial is being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct this was part of another change I was working on to add a drop-down with links to all the services to sub scribe to.
Unfortunately I think that needs to be its own PR. Those icons are from font awesome of which the theme uses Fork Awesome which does not yet include an icon for X. An update to that library would also warrant testing of all icons used on the site. This change fixes what is now a broken image on the site. |
date = "2022-07-23T03:51:55-04:00" | ||
draft = false | ||
logo = "https://upload.wikimedia.org/wikipedia/commons/4/4f/Twitter-logo.svg" | ||
logo = "https://upload.wikimedia.org/wikipedia/commons/c/ce/X_logo_2023.svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still using Wikipedia as a CDN, which might not be very reliable. The file name and path might get changed any time. For example, the Twitter logo has already been removed. Also, it might load slower than a local file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the idea of the local files wherever we can, the official twitter logo can be be downloaded here.
We could even reuse the image to potentially replace the twitter icon on the show pages as well. I don't know off hand if that would be simple or not, but might be worth looking into as well since Fork Awesome still has not updated their icons to include the new twitter logo.