website icon indicating copy to clipboard operation
website copied to clipboard

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

Open izma-mujeeb opened this issue 1 year ago • 6 comments

Fixes #6911

What changes did you make?

  • In the wins.js page, I replaced the image that was initially being used, with the image that used an svg tag. This image has a name of "icon-github.svg"

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

izma-mujeeb avatar Oct 08 '24 00:10 izma-mujeeb

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

github-actions[bot] avatar Oct 08 '24 00:10 github-actions[bot]

Availability: M > F ETA: EOD

codyyjxn avatar Oct 08 '24 01:10 codyyjxn

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

8alpreet avatar Oct 08 '24 19:10 8alpreet

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.

8alpreet avatar Oct 09 '24 01:10 8alpreet

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 avatar Oct 09 '24 16:10 codyyjxn

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?

t-will-gillis avatar Oct 19 '24 23:10 t-will-gillis

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 #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

roslynwythe avatar Nov 18 '24 23:11 roslynwythe

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

k-cardon avatar Dec 01 '24 15:12 k-cardon

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

mchait18 avatar Dec 10 '24 20:12 mchait18

@k-cardon Thank you for your patience. Please let me know if it works now.

izma-mujeeb avatar Jan 11 '25 03:01 izma-mujeeb

Great, thanks for making those changes! I confirmed that the GitHub logo svg is displaying correctly! Looks like everything is fixed now.

k-cardon avatar Jan 19 '25 22:01 k-cardon

Hi @roslynwythe, @8alpreet, @FamousHero, @codyyjxn - @izma-mujeeb made changes and resolved merge conflicts and the PR is ready for your re-review. Please provide ETA for your review or if you cannot re-review, please comment and remove yourself from the Reviewers section. Thanks!

jphamtv avatar Jan 31 '25 00:01 jphamtv

Review ETA: 11 Feb 2025 @ 8 PM (GMT) Availability: Daily @ 6 PM - 8 PM (GMT)

mugdhchauhan avatar Feb 11 '25 17:02 mugdhchauhan

Availability: Weekdays 5:30pm - 10pm PST ETA: EOD March 23

Cloid avatar Mar 21 '25 14:03 Cloid

Hi @izma-mujeeb - Could you let us know if you're still planning to work on this? If so, please share your ETA for completion. If we don't hear back, we'll unassign the issue so someone else can pick it up. Thanks!

jphamtv avatar May 02 '25 01:05 jphamtv

Closing as original dev is inactive, the original issue will be reassigned.

daras-cu avatar Jul 01 '25 04:07 daras-cu