-
Notifications
You must be signed in to change notification settings - Fork 96
Replace X with Bluesky in the site footer #1749
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
Conversation
prompted by a request from @danielskatz in the #website Slack channel Signed-off-by: Chad Dougherty <[email protected]>
Signed-off-by: Chad Dougherty <[email protected]>
Static preview here, but I also did a fair amount of testing in a local container preview environment. The urlcheck failures are some of the usual false positives and work for me. |
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.
Thanks for doing this @crd477! I can change this to approved shortly, but I had two comments I thought worth mentioning. The changes and functionality are great. I just wanted to see if it was worth talking about these comments before merging this in.
class="button button-icon"> | ||
<svg xmlns="http://www.w3.org/2000/svg" height="32" width="32" viewBox="0 0 512 512"><!--!Font Awesome Free 6.5.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2023 Fonticons, Inc.--><path fill="#741654" d="M389.2 48h70.6L305.6 224.2 487 464H345L233.7 318.6 106.5 464H35.8L200.7 275.5 26.8 48H172.4L272.9 180.9 389.2 48zM364.4 421.8h39.1L151.1 88h-42L364.4 421.8z"/></svg> | ||
<span class="screen-reader-text">Twitter</span> | ||
<svg xmlns="http://www.w3.org/2000/svg" height="32" width="32" viewBox="0 0 512 512"><!--!Font Awesome Free 6.7.2 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2025 Fonticons, Inc.--><path fill="#741654" d="M111.8 62.2C170.2 105.9 233 194.7 256 242.4c23-47.6 85.8-136.4 144.2-180.2c42.1-31.6 110.3-56 110.3 21.8c0 15.5-8.9 130.5-14.1 149.2C478.2 298 412 314.6 353.1 304.5c102.9 17.5 129.1 75.5 72.5 133.5c-107.4 110.2-154.3-27.6-166.3-62.9l0 0c-1.7-4.9-2.6-7.8-3.3-7.8s-1.6 3-3.3 7.8l0 0c-12 35.3-59 173.1-166.3 62.9c-56.5-58-30.4-116 72.5-133.5C100 314.6 33.8 298 15.7 233.1C10.4 214.4 1.5 99.4 1.5 83.9c0-77.8 68.2-53.4 110.3-21.8z"/></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.
I know this isn't necessarily the right PR for this, but does anyone know why we are using these complicated svgs with path, rather than the fa-brands fa-bluesky
, something like <i class="fab fa-bluesky" aria-hidden="true"></i>
. Do we not load the Font Awesome stylesheet at all and so the icons are not available?
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 know what, just ignore me. There isn't any reason not to really use the svg. They are in theory better since we could be handling arbitrarily sized screens. I just personally never thought about using the svgs for small icons like this, since you have to essentially go to the FA website to get the right path values, making them a little harder to work with FA icons when using them throughout a website.
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.
Ugh sorry, actually before we resolve this. I did notice we have one more file to consider. We are not using the svgs anywhere else on this website (which is fine). But more specifically, we do have a "share button" file we'll need to add fa-bluesky to, to replace twitter. _includes/buttons/share-buttons.html. I need to check to make sure how this is used, because we likely need to update the link for this as well...
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 know what, just ignore me. There isn't any reason not to really use the svg. They are in theory better since we could be handling arbitrarily sized screens. I just personally never thought about using the svgs for small icons like this, since you have to essentially go to the FA website to get the right path values, making them a little harder to work with FA icons when using them throughout a website.
My interpretation, as you mentioned already, is that we do not load the whole FA stylesheet, so we use the SVGs instead. I read that loading the SVGs as we do is more performant, but I have no firsthand knowledge on the subject.
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.
Ugh sorry, actually before we resolve this. I did notice we have one more file to consider. We are not using the svgs anywhere else on this website (which is fine). But more specifically, we do have a "share button" file we'll need to add fa-bluesky to, to replace twitter. _includes/buttons/share-buttons.html. I need to check to make sure how this is used, because we likely need to update the link for this as well...
I mentioned this in the Slack channel, and I was kinda thinking that it's a separate issue, since the share buttons have other things that aren't in the footer now, like Facebook and Reddit. I'm totally OK with changing them in this PR though.
color: #fff; | ||
} | ||
.btn--bluesky:hover { | ||
background-color: #006292; |
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.
In the preview, I'm not seeing this color change in any meaningful way when hovering over any of the footer icons. Maybe an issue to track for another PR?
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.
Actually to be a bit more specific, I don't see these color changes in the live site either.
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.
Actually to be a bit more specific, I don't see these color changes in the live site either.
Hrm. I'm not sure what's going on with that. I was just mimicking the existing buttons.
Should we update the "newer" newsletters to have the BlueSky link instead? I was updating the newsletter template at least, since that still has twitter link in it. I'm not sure how far we should go back, or just not at all and just have it updated for the future newsletters. |
@crd477 I have a few changes I can push to your branch, but don't have access to your fork (obviously). I'm not sure what's the best way to try to get these changes to you. This is honestly the first time I've tried to commit to a PR of someone else's fork for a repo. I'm not sure if there is a more appropriate way to commit directly to this repo, rather than yours. If there is, just let me know. Otherwise, I might need access to commit to your repo/branch, or maybe I have to open a PR to yours? Or I could try to make the direct suggestions in this PR as comments on here. Technically, these could also be another branch/PR entirely, if you'd prefer this to just be fixing the footer and not trying to resolve "most" twitter/X things. The files I had to touch were head.html (for Font Awesome 5->6 update, since the BlueSky icon isn't available in 5), reboot.css (to fix the share buttons and another Font Awesome 5 reference), share-buttons.html (to replace twitter with bluesky), and newsletter-template.md (to update the Twitter link in there). I haven't tried to update any previous newsletters like mentioned above. |
I've never done that either. I will see if I can add access for you.
I don't feel strongly either way. I was mainly just doing what I felt was the minimum right now. |
Err, nevermind. I pushed the same branch to the US-RSE repo. I think you can work on it there. Closing this PR in lieu of that... |
Description
This change replaces the Twitter/X icon in the site footer with Bluesky. It is motivated by a request from @danielskatz on the #website Slack channel.
Checklist:
When you are ready for a technical review/merge, post the for the link for the PR in the US-RSE Slack (#website) to ask for reviewers.