Update Twitter in footer to X#35443
Update Twitter in footer to X#35443slavingia wants to merge 3 commits intodepartment-of-veterans-affairs:mainfrom
Conversation
Update header-footer-data.json to have X vs Twitter
|
@slavingia Is there a ticket associated with this work? |
|
No, I don't have access to Jira etc. But I think this is an easy, small change we should make. Happy to chat over Teams and discuss live if you disagree. |
|
Hi @slavingia |
|
What does this file do, then? Should it be deleted, as it seems duplicative to Drupal? Btw, I think we should consider removing Drupal as part of our workflow, and all content should just live in the codebase. |
I do have an objection. It's a bit strange to say "formerly known as Twitter", and I think we can address the touch target differently if it's a real problem. Here's an example of small touch targets on another federal government website:
|
|
The file that was changed is not an auto-generated file. It is a hardcoded file that is only used when engineers are running vets-website for local development (without content-build) and should not change prod if it is merged. Because engineers use it for local development, please do not delete it. Implementing social media icons is in our backlog but has not yet been prioritized. As an alternative, would X.com be an acceptable replacement? |
@slavingia - this is how it used to be, and was moved into Drupal for strategic reasons. Happy to talk through this more, though this PR seems not the right forum. |
|
Sure I'll ping you on Teams. And no, X.com isn't an acceptable replacement. It must be "X" to be consistent with the other sites where we use the name they prefer. |
|
Even if it's development only, still good to change to match production? |
|
Thank you - the link in the footer has been changed to X as requested and will be reflected on prod following the next content release. |
|
Woot thank you! Hopefully we can merge this in too so developers see the change locally. Appreciated |
|
@mmiddaugh following up on merging this! |
|
@slavingia You will need to get your CI passing before you will be able to merge; there are some failures and pending checks. Might need to update with |
|
It's probably because I don't have AWS creds etc. |
|
I'd recommend pulling it yourself and running and seeing if the tests pass for you. |
|
@slavingia You will not be able to merge unless your CI is passing. Whether or not it works locally is not relevant, unfortunately - these CI checks are in place to be sure the repository is in good shape before merging new code. You don't need AWS credentials to get the CI to run. There are controls within this PR that you can use to re-run checks, or you can pull in |
|
Could you look at the specific failures and let me know how to fix? They are not related to the code change and imo flakey tests shouldn't block merging. Only a review is required. |
|
This PR is failing because it is attempting to merge from a different repo. For this request, you should clone the repo, make the change on a feature branch, push the feature branch to the vets-website repo, and create the PR to main from there. Thanks. |
|
Can you do that? Would be faster than telling me to. |
|
PR #35492 took care of this and is merged. It will be deployed to the site with our daily scheduled deployment, that kicks off at 1pm ET today. |
|
Thank you very much! |
6 participants
What
Updating Twitter name to match the new site name (X).
Why
Follows what people (veterans, etc) expect.