website icon indicating copy to clipboard operation
website copied to clipboard

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

Open roslynwythe opened this issue 1 year ago • 6 comments

Overview

We need to update the code that renders the GitHub icon and badges on the Wins page, to display the GitHub graphic file _includes/svg/icon-github.svg, and also to remove the graphic files that are currently used on the Wins page. These changes will eliminate duplicate GitHub graphic files, resulting in a cleaner codebase.

Action Items

  • [ ] Update assets/js/wins.js so that all instances of the GitHub icon reference the file _includes/svg/icon-github.svg
  • [ ] Note that there are multiple locations in which GitHub icons are referenced, due to the fact that the wins cards display different markup in mobile view vs. desktop view, and the cards can be "expanded" after page load, and the GitHub icon appears in both the context of the developer's badges ("I increased the number of commits on my GitHub profile") as well as an icon representing the developer's GItHub profile
    • [ ] Note that the fill color of the badges is different than the color of the standard GitHub icon. Refer to the wiki page #6654 for guidance
  • [ ] Remove the file currently used graphic files /assets/images/wins-page/icon-github-small.svg and assets/images/wins-page/wins-badges/github.svg
  • [ ] Use Docker to preview the Wins page (/wins) to ensure that the appearance and behavior of the Wins page is unchanged both in mobile and desktop views.

Resources/Instructions

roslynwythe avatar May 29 '24 09:05 roslynwythe

Hi @roslynwythe.

Please don't forget to add the proper labels to this issue. Currently, the labels for the following are missing:

  • Size

NOTE: Please ignore this comment if you do not have 'write' access to this directory.

To add a label, take a look at Github's documentation here.

Also, don't forget to remove the "missing labels" afterwards. To remove a label, the process is similar to adding a label, but you select a currently added label to remove it.

After the proper labels are added, the merge team will review the issue and add a "Ready for Prioritization" label once it is ready for prioritization.

Additional Resources:

github-actions[bot] avatar May 29 '24 09:05 github-actions[bot]

Hi @tony1ee, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

github-actions[bot] avatar Jun 08 '24 01:06 github-actions[bot]

i. Availability: 06/07/2024 7-10 PM 06/08/2024 9-11 AM ii. ETA: 06/08/2024 EOD

tony1ee avatar Jun 08 '24 01:06 tony1ee

@tony1ee

Please add update using the below template (even if you have a pull request). Afterwards, remove the '2 weeks inactive' label and add the 'Status: Updated' label.

  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (optional): "Add any pictures of the visual changes made to the site so far."

If you need help, be sure to either: 1) place your issue in the Questions/In Review column of the Project Board and ask for help at your next meeting, 2) put a "Status: Help Wanted" label on your issue and pull request, or 3) put up a request for assistance on the #hfla-site channel. Please note that including your questions in the issue comments- along with screenshots, if applicable- will help us to help you. Here and here are examples of well-formed questions.

You are receiving this comment because your last comment was before Tuesday, June 25, 2024 at 12:06 AM PST.

github-actions[bot] avatar Jun 28 '24 07:06 github-actions[bot]

Progress:

  1. analyzed win.js and identified 3 cases in total where /assets/images/wins-page/icon-github-small.svg or assets/images/wins-page/wins-badges/github.svg was used:
  2. made changes so references to /assets/images/wins-page/icon-github-small.svg is now referencing icon-github.svg
  3. realized changing the remaining reference to assets/images/wins-page/wins-badges/github.svg is not as straightforward and need some logic changes, see Blockers
  4. read similar issues for the slack svg update but no useful info was found, since this is a case only for the GitHub icon.

Blockers: in the following (line 28) reference to assets/images/wins-page/wins-badges/github.svg https://github.com/hackforla/website/blob/e43e20a3dfb9e676abb8b24dedda0afa972e29de/assets/js/wins.js#L23-L39 only the filename is referenced, and the path is then assembled in insertIcons() defined in line 245, specifically the following lines: https://github.com/hackforla/website/blob/e43e20a3dfb9e676abb8b24dedda0afa972e29de/assets/js/wins.js#L264-L280

so this reference cannot be changed without setting exceptions in insertIcons(), but hardcoding an exception it in insertIcons() seems not ideal.

Another possible way is to include the full path in badgeIcons defined in line 23, move the SVG_FILE_PATH definition to the outside insertIcons() and define every other icon as

<win_description>: SVG_FILE_PATH + `<filename>.svg`

and for the GitHub win:

"I increased the number of commits on my Github profile": `_includes/svg/icon-github.svg`

I would like to get further guidance on the proper way to implement this change.

Availability: 4 hours over this weekend ETA: pending further guidance before continuing

tony1ee avatar Jun 29 '24 01:06 tony1ee

Thank you @tony1ee for taking this issue and for your detailed description of the challenge regarding the replacement of assets/images/wins-page/wins-badges/github.svg. Since this is a Complexity: Medium issue, I'll limit the scope to updating references to /assets/images/wins-page/icon-github-small.svg to icon-github.svg, and we will tackle the question of assets/images/wins-page/wins-badges/github.svg separately. You did a great job of describing some options. Another option would be to leave references to assets/images/wins-page/wins-badges/github.svg as they are, and accept that there is a duplicate image. That should be a discussion with the PM. But for now I believe you can submit your PR. Please send me a Slack DM when it is ready to review

roslynwythe avatar Jul 01 '24 05:07 roslynwythe

Thanks @roslynwythe. Will ignore the reference to assets/images/wins-page/wins-badges/github.svg for now.

For the other references, upon testing I realize I cannot change the path directly to /_includes/svg/icon-github.svg, since files in the /_includes/ directory are not directly accessible via URL and a 404 error will occur:

ERROR `/_includes/svg/icon-github.svg' not found.

I am not sure how I could change references to files in /_includes/ within a JS file using Liquid template. No other issue in #6903 (GitHub icon) or #5758 (Slack icon) involves changing a reference in a .js file.

Some further guidance requested.

tony1ee avatar Jul 08 '24 02:07 tony1ee

@tony1ee You are correct, the img tag cannot reference /_includes/svg/icon-github.svg, but in place of the img tag, you can use liquid to "include" the svg inline. See https://github.com/hackforla/website/issues/6653#issuecomment-2086713272 in particular the sections "Use an ARIA label in the div HTML tag wrapping the SVG" and "Use an outer SVG with aria-label or aria-labelledby".

I updated the body of the issue for clarification.

roslynwythe avatar Jul 08 '24 19:07 roslynwythe

@tony1ee

Please add update using the below template (even if you have a pull request). Afterwards, remove the '2 weeks inactive' label and add the 'Status: Updated' label.

  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (optional): "Add any pictures of the visual changes made to the site so far."

If you need help, be sure to either: 1) place your issue in the Questions/In Review column of the Project Board and ask for help at your next meeting, 2) put a "Status: Help Wanted" label on your issue and pull request, or 3) put up a request for assistance on the #hfla-site channel. Please note that including your questions in the issue comments- along with screenshots, if applicable- will help us to help you. Here and here are examples of well-formed questions.

You are receiving this comment because your last comment was before Tuesday, July 30, 2024 at 12:05 AM PST.

github-actions[bot] avatar Aug 02 '24 07:08 github-actions[bot]

@tony1ee

Please add update using the below template (even if you have a pull request). Afterwards, remove the '2 weeks inactive' label and add the 'Status: Updated' label.

  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures (optional): "Add any pictures of the visual changes made to the site so far."

If you need help, be sure to either: 1) place your issue in the Questions/In Review column of the Project Board and ask for help at your next meeting, 2) put a "Status: Help Wanted" label on your issue and pull request, or 3) put up a request for assistance on the #hfla-site channel. Please note that including your questions in the issue comments- along with screenshots, if applicable- will help us to help you. Here and here are examples of well-formed questions.

You are receiving this comment because your last comment was before Tuesday, August 6, 2024 at 12:04 AM PST.

github-actions[bot] avatar Aug 09 '24 07:08 github-actions[bot]

Have to take a personal break for a few weeks, unassigning myself from this issue so other developers can work on this.

tony1ee avatar Aug 15 '24 14:08 tony1ee

Hello @dcotelessa, we appreciate you taking on this issue, however it looks like you're already working on another issue at this time. Please wait until your current issue is merged before taking on another issue. :)

We are going to unassign you from this issue so you can focus on your current issue.

Hfla appreciates you! :)

HackforLABot avatar Aug 18 '24 17:08 HackforLABot

@roslynwythe @t-will-gillis this is not ready to prioritize yet. Because of the comments with back and forth between Tony and Roslyn it's no longer clear what to do. I am adding this to our next team meeting, but you @roslynwythe You can review it and revise before then if you want.

ExperimentsInHonesty avatar Aug 27 '24 01:08 ExperimentsInHonesty

Hello @terrencejihoonjung, we appreciate you taking on this issue, however it looks like you're already working on another issue at this time. Please wait until your current issue is merged before taking on another issue. :)

We are going to unassign you from this issue so you can focus on your current issue.

Hfla appreciates you! :)

HackforLABot avatar Sep 14 '24 23:09 HackforLABot

ETA: 9/18/24, 11:59pm PST Availability: any day, 3pm - 9pm PST

terrencejihoonjung avatar Sep 14 '24 23:09 terrencejihoonjung

Hi @izma-mujeeb, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

HackforLABot avatar Oct 01 '24 00:10 HackforLABot

Availability: Mon-Fri 5PM - 9PM ETA: Wednesday, October 2

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

I have a blocker. I am trying to change the icon to include the svg file, however, every time I change the file, I get this error: ERROR '/wins/_includes/svg/icon-github.svg' not found. I am not sure why this error is coming up as the above is not a path that I am including in the JavaScript file. As well, since the svg file is under the _includes folder, I don't think these images can be used with an img tag, as these files are treated as static assets.

izma-mujeeb avatar Oct 16 '24 21:10 izma-mujeeb

@izma-mujeeb @t-will-gillis I have tried prepending the file path, _includes/svg/icon-github.svg, with each of: /wins/, /, and ../../ and none resolved the issue. All still give the same error message:

ERROR `/wins/_includes/svg/icon-github.svg' not found.

Update

After prepending the file path with ../../ and using tab completion to finish the path, I get a new error:

`/_includes/svg/icon-github.svg' not found.

DrAcula27 avatar Oct 30 '24 02:10 DrAcula27

At the office hour today, we looked through other files that use svg images in a JavaScript file, and tested to see if the following URL would work: {% include svg/icon-github.svg %}. Unfortunately, this link did not work and the Github icon is not being displayed on the Wins page.

izma-mujeeb avatar Nov 01 '24 02:11 izma-mujeeb

These are the svgs that are in _includes folders https://github.com/hackforla/website/tree/gh-pages/_includes/svg

Here is the search I used

  • https://github.com/search?q=repo%3Ahackforla%2F[REPLACE WITH NAME OF FILE]+language%3AJavaScript+&type=code
  • example of not found: https://github.com/search?q=repo%3Ahackforla%2Fwebsite+toolkit-hero.svg+language%3AJavaScript+&type=code
  • example of file found in a JavaScript file: https://github.com/search?q=repo%3Ahackforla%2Fwebsite+icon-hamburger-nav.svg+language%3AJavaScript+&type=code

Here is the JavaScript file that has references to svgs that are in the _includes folder https://github.com/hackforla/website/blob/gh-pages/assets/js/hamburger-nav.js https://github.com/hackforla/website/blob/9115924e1427c62d78df14773799de6fa4f9f9c9/assets/js/hamburger-nav.js#L14-L16 https://github.com/hackforla/website/blob/9115924e1427c62d78df14773799de6fa4f9f9c9/assets/js/hamburger-nav.js#L34-L39

ExperimentsInHonesty avatar Nov 01 '24 17:11 ExperimentsInHonesty

At the office hour today, we looked through other files that use svg images in a JavaScript file, and tested to see if the following URL would work: {% include svg/icon-github.svg %}. Unfortunately, this link did not work and the Github icon is not being displayed on the Wins page.

  • @izma-mujeeb I believe that {% include svg/icon-github.svg %} is the correct liquid statement to reference the svg. If it is not working, I'll need to see the surrounding js and html code. Previously the HTML was using an img tag and that will have to be changed. For the purposes of styling and accessibility, we will probably want to wrap the svg in an outer svg. You can find sample code for that in https://github.com/hackforla/website/issues/6653#issuecomment-2086713272 which is listed as a resource item in this issue. If you wish, you can create a Draft PR so that I can see your code.

roslynwythe avatar Nov 03 '24 18:11 roslynwythe

@izma-mujeeb please submit a draft pr. You submit a PR as normal and then change it to draft by clicking on the link that looks like this, under the assignees. Once you done that, stick this issue back into the questions column with a ready for dev lead label

image

ExperimentsInHonesty avatar Nov 05 '24 01:11 ExperimentsInHonesty

@ExperimentsInHonesty @roslynwythe I submitted a PR and have moved this issue back into questions column with the ready for dev lead label. https://github.com/hackforla/website/pull/7569

izma-mujeeb avatar Nov 05 '24 01:11 izma-mujeeb

@roslynwythe spoke with @izma-mujeeb and revised the issue to make it clearer and reduce scope. See details in PR

ExperimentsInHonesty avatar Nov 19 '24 01:11 ExperimentsInHonesty

@ExperimentsInHonesty I rewrote this issue so the action items are more specific since the previous dev had some trouble, I believe it should be ready for re-prioritization.

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

Hi @santi-jose, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

HackforLABot avatar Oct 14 '25 05:10 HackforLABot

i. Availability: Monday-Friday 5PM-8PM ii. ETA: Friday 10/24/25

santi-jose avatar Oct 14 '25 05:10 santi-jose