Update wins.js so the Wins page uses icon-github.svg #7569

izma-mujeeb · 2024-10-08T00:22:50Z

Fixes #6911

What changes did you make?

In the wins.js page, I updated the makeCards function by replacing the image that was initially being used, with the image in the svg tag. This image has a name of "icon-github.svg". Since svgs files do not support the alt attribute, I had to update the html in _includes/wins-page/wins-card-template.html/wins-card-template.html to ensure the svg file was displaying on the webpage.

Why did you make the changes (we will use this info to test)?

I had to change the image because using the original image was causing a redundancy in the number of images. By using an image with the svg tag, I am ensuring that there are no duplicate images.

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

No visual changes made to the website

github-actions · 2024-10-08T00:23:22Z

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.

git checkout -b izma-mujeeb-update-wins.js-to-include-svg gh-pages
git pull https://github.com/izma-mujeeb/website.git update-wins.js-to-include-svg

codyyjxn · 2024-10-08T01:40:11Z

Availability: M > F
ETA: EOD

codyyjxn

Hi @izma-mujeeb Thank you for taking on this issue!

The branch name is correct but I recommend you include the issue number for future reference.
The issue is linked correctly
The changes are correct I checked on my end I was able to see the changes using docker.
The title and description are accurate to understand what the task/change.

Great Job! Thanks for your contribution.

8alpreet · 2024-10-08T19:47:47Z

Availability: 3-7pm, Mon-Fri
ETA: EOD Tue, Oct 8

8alpreet · 2024-10-09T01:18:31Z

I am testing these changes locally and the icon is not displaying for me. Does it display for both of you @izma-mujeeb and @codyyjxn?

I'm still doing research, but all other instances of the _includes/svg/icon-github.svg use the include tag or are within a .yml file (which I don't know the internals of). I haven't found any instances of a .svg from _includes being used with an img tag. Jekyll does not treat files in the _includes directory as static assets, so we should not be able to use it as the value of the src attribute in an image.

codyyjxn · 2024-10-09T16:08:21Z

I am testing these changes locally and the icon is not displaying for me. Does it display for both of you @izma-mujeeb and @codyyjxn?

I'm still doing research, but all other instances of the _includes/svg/icon-github.svg use the include tag or are within a .yml file (which I don't know the internals of). I haven't found any instances of a .svg from _includes being used with an img tag. Jekyll does not treat files in the _includes directory as static assets, so we should not be able to use it as the value of the src attribute in an image.

Hey @8alpreet yea just check and for some reason I cant see the icon either. I think maybe the svg isn't rendering correctly.

codyyjxn

Hey @izma-mujeeb I was looking into the code and @8alpreet actually brought up a good point but we arent able to see the github icon. Please provide screenshots of what you are seeing on your end. The before and after you fix the code. Thank you ! If you have any question feel free to message any of us.

k-cardon

In case it's helpful--here's the error message that came up when I ran the docker container locally:

hfla_site | [2024-10-11 04:41:18] ERROR '/wins/_includes/svg/icon-github.svg' not found.

I also can't see the icon, so there's definitely a bug. Is wins/_includes the correct path?

t-will-gillis · 2024-10-19T23:05:07Z

Hey @izma-mujeeb @8alpreet @k-cardon @codyyjxn I took a stab at figuring this out also- I am getting no further than you are. The path _includes/svg/icon-github.svg appears correct.

I do not know Liquid, but I am thinking that maybe(?) Liquid wants these assets to be in the /assets/ folder. There is the footer.html that uses the same GitHub icon. I am not certain but to me it doesn't look like there is a simple 'swap-out-the-new-path' substitution.

@roslynwythe do you know more about this?

FamousHero

Hi @izma-mujeeb, thanks for taking on this issue!

In terms of pull request formatting, everything looks great! I do agree with @codyyjxn that the issue number should be included on the branch name but besides that there are no issues. Like others have said, I'm having issues seeing the changes on my machine. I'm unsure what the exact issue is but Jekyll has great documentation and if not mistaken should have a section on static assets and includes

Jekyll Docs

roslynwythe · 2024-11-11T23:51:57Z

assets/js/wins.js

@@ -330,7 +330,7 @@

 		if (card[github_url].length > 0){
 			cloneCardTemplate.querySelector('.wins-card-github-icon').href = card[github_url];
-			cloneCardTemplate.querySelector('.github-icon').src = GITHUB_ICON ;
+			cloneCardTemplate.querySelector('.github-icon').src = GITHUB_ICON;


@izma-mujeeb The liquid statement {% include svg/icon-github.svg %} will result in an inline svg, which should replace the existing img tag. To accomplish this, I think you will want to make changes to both wins.js and the html template (_includes/wins-page/wins-card-template.html/wins-card-template.html). I should have mentioned that in the issue. I added an Action Item and Resources to the issue, to clarify.

Part of the issue is making the svg accessible. The Resource making SVGs WCAG compliant provides sample code for doing that.

We could setup a Zoom meeting to go over this, or we could discuss it in the Thursday office hours. Please send me a Slack message and let me know your availability. If you would like to meet, please send me a Slack message.

roslynwythe · 2024-11-18T23:57:34Z

The issue has been edited to:

reduce scope to replacing the GitHub icons in the wins cards, which appear on page load. When the wins card is expanded ("See More"), an overlay appears. Initially this issue was intended to address the wins card and the overlay, but a new issue ER: Update wins.js so the Wins overlay uses icon-github.svg #7727 has been created to address the overlay.
include a reference with sample code for making the svg accessible
include mention and references to _includes/wins-page/wins-card-template.html/wins-card-template.html

k-cardon · 2024-12-01T15:47:14Z

@izma-mujeeb could you resolve the conflicts in assets/js/wins.js?

mchait18 · 2024-12-10T20:03:04Z

Review ETA: 12 PM 12/10/24
Availability: 9-3 EST mon-thurs

update wins.js so the Wins page uses icon-github.svg

cf6a2aa

github-actions bot added role: front end Tasks for front end developers Complexity: Medium P-Feature: Wins Page https://www.hackforla.org/wins/ Feature: Refactor HTML size: 1pt Can be done in 4-6 hours labels Oct 8, 2024

codyyjxn self-requested a review October 8, 2024 01:38

codyyjxn approved these changes Oct 8, 2024

View reviewed changes

FamousHero self-requested a review October 8, 2024 18:27

8alpreet self-requested a review October 8, 2024 19:47

8alpreet mentioned this pull request Oct 9, 2024

Skills Issue: Developer: Balpreet Hehar #7205

Open

codyyjxn requested changes Oct 9, 2024

View reviewed changes

k-cardon self-requested a review October 11, 2024 02:27

k-cardon requested changes Oct 11, 2024

View reviewed changes

k-cardon mentioned this pull request Oct 11, 2024

Skills Issue: Developer: Kristen Cardon #7326

Open

daras-cu marked this pull request as draft October 16, 2024 02:38

FamousHero requested changes Oct 22, 2024

View reviewed changes

FamousHero mentioned this pull request Oct 22, 2024

Skills Issue: Developer: Daniel Franco #7327

Open

fix svg file

bc70186

izma-mujeeb mentioned this pull request Nov 5, 2024

Update wins.js so the Wins page uses icon-github.svg #6911

Open

7 tasks

roslynwythe self-requested a review November 11, 2024 23:33

roslynwythe requested changes Nov 11, 2024

View reviewed changes

fixed html and svg files to ensure icon is being display properly

0cabf1e

izma-mujeeb marked this pull request as ready for review November 28, 2024 02:31

izma-mujeeb requested review from codyyjxn, k-cardon, FamousHero and roslynwythe November 28, 2024 02:34

mchait18 self-requested a review December 10, 2024 20:02

mchait18 removed their request for review December 13, 2024 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wins.js so the Wins page uses icon-github.svg #7569

Update wins.js so the Wins page uses icon-github.svg #7569

izma-mujeeb commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024

codyyjxn commented Oct 8, 2024

codyyjxn left a comment

8alpreet commented Oct 8, 2024

8alpreet commented Oct 9, 2024 •

edited

Loading

codyyjxn commented Oct 9, 2024

codyyjxn left a comment •

edited

Loading

k-cardon left a comment •

edited

Loading

t-will-gillis commented Oct 19, 2024

FamousHero left a comment

roslynwythe Nov 11, 2024 •

edited

Loading

roslynwythe commented Nov 18, 2024 •

edited

Loading

k-cardon commented Dec 1, 2024

mchait18 commented Dec 10, 2024

Update wins.js so the Wins page uses icon-github.svg #7569

Are you sure you want to change the base?

Update wins.js so the Wins page uses icon-github.svg #7569

Conversation

izma-mujeeb commented Oct 8, 2024 • edited Loading

What changes did you make?

Why did you make the changes (we will use this info to test)?

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

github-actions bot commented Oct 8, 2024

codyyjxn commented Oct 8, 2024

codyyjxn left a comment

Choose a reason for hiding this comment

8alpreet commented Oct 8, 2024

8alpreet commented Oct 9, 2024 • edited Loading

codyyjxn commented Oct 9, 2024

codyyjxn left a comment • edited Loading

Choose a reason for hiding this comment

k-cardon left a comment • edited Loading

Choose a reason for hiding this comment

t-will-gillis commented Oct 19, 2024

FamousHero left a comment

Choose a reason for hiding this comment

roslynwythe Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

roslynwythe commented Nov 18, 2024 • edited Loading

k-cardon commented Dec 1, 2024

mchait18 commented Dec 10, 2024

izma-mujeeb commented Oct 8, 2024 •

edited

Loading

8alpreet commented Oct 9, 2024 •

edited

Loading

codyyjxn left a comment •

edited

Loading

k-cardon left a comment •

edited

Loading

roslynwythe Nov 11, 2024 •

edited

Loading

roslynwythe commented Nov 18, 2024 •

edited

Loading