Fix #7112: Update 'Github' to 'GitHub' in social.yml#8018
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. |
|
Review ETA: 1:45 PM EST 3/26/25 |
|
Review ETA: Wednesday 03/26 2:30 PST |
nchanyal
left a comment
There was a problem hiding this comment.
Hey @ihop-56.
First of all, thank you for making this PR. You did well following the instructions, but I have a few changes I would like you to make before I approve.
I would say the biggest change you can make is with the test procedure. Right now it isn't clear to me. The reason being is that you say that there are no visual changes to the website but ask us to check that the label in the header and footer now says 'GitHub' instead of 'Github.' I need clarification here.
But, assuming there are visual changes, I don't see them in both the header and footer. When I checked the header, there wasn't any label with 'Github' nor 'GitHub.' I also checked the footer but didn't see any labels.
You'll also want to include before and after screenshots of the website if you decide that visual changes were made. I understand you're having difficulty with Docker and would suggest you go to office hours to get that sorted. Viewing the changes you made yourself helps us reviewers complete our reviews faster.
This is relatively minor, but I want to bring it up because it's important that we follow the action items in the original issue. You did a great job documenting the test procedure in this PR; however, you should also comment on it in the original issue and include a link in this PR as the original issue asks. Here's the quote if you're wondering:
'Write a test procedure, in a comment, and then use it to test the change. Include a link to this comment in your PR'.
If you have any questions about what I said, feel free to ask.
There was a problem hiding this comment.
Hello @ihop-56
Thank you for taking this issue. Your contribution is greatly appreciated.
You did an excellent job following the instruction of your issue. You made the correct changes in the codebase without breaking the GitHub link.
Some suggestions:
Suggestion #1:
In your Why did you make the changes section, the third bullet point is not fully true. The footer links will render properly with the correct name (e.g GitHub), that is correct. However, when you say footer and header that is incorrect because there is no social media links for the header. In the social.yml file GitHub header element is set to false meaning there will not be any GitHub links on any header webpages. Just remove header from the third bullet point to make the entirely of your explanation true. Beside that good explanation.
Suggestion #2:
In your Test Plan section in step 3; again it is not entirely true because there is no social media link for your header. Furthermore, Step 4 is ambiguous because like you said in your comment session in the original issue “No visual changes expected; just a text update from “Github” to “GitHub”, so in order for the user to see that change, they would need to use their browser developer tool. A developer-friendly explanation would be something of the line of:
Confirm the label shows GitHub by:
- Right-clicking on the GitHub icon link (bottom of the webpage).
- Select inspect from the list.
- In your developer tool select the
Elementstab. From there you’ll see a line<span class="sr-only”>GitHub</span>confirm that the content between the<span>tag isGitHubnotGithub
Suggestion #3
In the Test Notes section I see that you’re unable to run docker locally, there can be many reasons for that, but definitely bring this up at one of the meetings. Furthermore, your explanation is not entirely true. Even though both are getting data from data/navigation/social.yml that does not mean {{ item.name }} is rendering the correct data on the webpage. In matter of fact only one of those files is relavant and being used. That file is _includes/footer.html. Check out website/_layouts/default.html file and in that file you'll find a line that looks like this: {%- include footer.html -%} what that is saying is on any webpage located at the footer of the page use footer.html to rendered all the data that are in data/navigation/social.yml That is the reason why you see the links at the footer of each page. As for _includes/social.html (for header) you can remove that from your explanation, becuase it does not have any bearing or use cases on your issue. Again, your social media links will not be displayed on any webpages headers.
I do want to confirm that you did everything right and GitHub is properly showing on the webpage. Just remove everything with headers and I think you'll be good for approval.
Great job on your PR! Just re-request me when you are finish.
|
Review ETA: 1 PM 4/1/25 |
There was a problem hiding this comment.
Hi all.
Thanks for taking up this issue Rak @ihop-56
I've tested thoroughly and found that everything works correctly, inspecting in the process.
First off, @ihop-56 update on if you could make Docker work and if you need help just Slack I can help 👍
For this issue it's very useful to use a global search on vsCode
Press Cmd + Shift + F (on Mac) or Ctrl + Shift + F (on Windows/Linux) to open global search.
I've been reading the previous reviews. I think that this issue should have had a bit more points, but anyways I agree that there should be a bit more documentation done and testing instructions/documentation as @dvernon5 @nchanyal mention. I would add that an explanation on what each files does/represents could be added also.
The issue instructions are more open because it's a medium issue, which is more complex, so there's much more stuff to research and gaps to fill. Many of us are jumping at this type of issue without having previous knowledge, so if we all provide a simple explanation it's very valuable.
social.yml contains the social media links of Hack for LA. I'll be redundant here but to clarify, the whole issue is about how changing this file affects other files. So knowing what those files are is important. The main idea is to learn how the website renders and works, even if the change is simple and not that important, ensuring that nothing breaks is very important.
Apparently footer.html that renders the data in social.yml is the main dependency. Though I noticed there's at least another file called social.html that uses it.
Then all files using footer.html are also affected by the change in social.yml
What's changed in the website while inspecting is
<a href="https://github.com/hackforla" rel="noopener" target="_blank" class="js-inline-link js-inline-link-github" aria-label="Github">
and for screen readers
<span class="sr-only">Github</span>
</a>
Please let me know if I got the idea right, this is what I understood 👍
|
Hi @dvernon5 @nchanyal @santisecco Thank you all for your thoughtful feedback! ✅ I've removed all references to the header since the GitHub social link is not displayed there ( ✅ The Why section and Test Plan are now updated to reflect that the change affects only the footer, not both header and footer. ✅ The Test Plan now includes a clearer explanation for checking the updated text using browser dev tools:
✅ I’ve also added a comment with the test procedure on the original issue and included the link in the PR, as requested. Let me know if there's anything else I should address. Appreciate your time reviewing! Best, |
Hi @ihop-56,
Thanks for making the changes. I'm currently on hiatus so I can't check the changes you made but I believe the other reviewers can step in to do so.
|
@nchanyal Nathnael please review the new changes |
|
@ihop-56 Well done Rak you can move on to another issue |
Please note: You must be a member of the HFLA website team in order to create pull requests. Please see our page on how to join us as a member at HFLA: https://www.hackforla.org/getting-started. Delete this message if you joined this team via onboarding.
Fixes #7112
What changes did you make?
_data/navigation/social.ymlto changename: Github→name: GitHubto reflect correct branding.Why did you make the changes (we will use this info to test)?
site.data.navigation.social.CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website
sr-onlyin the site footer.Test Plan
Steps:
docker compose upor preview in deployment.http://localhost:4000.Test Notes
Unable to run Docker locally on my machine. As an alternative, I manually reviewed:
_data/navigation/social.yml: Contains the social media metadata.
_includes/footer.html: Renders the footer and pulls from the above data using {{ item.name }}.
_layouts/default.html: Includes footer.html on every page of the site.
Because footer.html uses the name field in social.yml, the text update from “Github” to “GitHub” should be accurately reflected site-wide in the footer.
Please help confirm through local testing or deployment preview.