Update wins.html file to use liquid in this file #5258

ronaldpaek · 2023-08-19T09:34:20Z

Part of #1442 (this pr should not close that issue)

Changes Made:

I adjusted the wins.html file to handle the liquid logic internally rather than within the JS file. This involved creating two divs set to "display none" and leveraging the data property to store data using the liquid attribute.
In the wins.js file, the primary modifications were renaming some variables and using document.querySelector to select the DOM node.

Reason for Changes:

The primary motivation behind these changes was our future intention to incorporate eslint. My objective was to rectify numerous syntax errors and red underlines appearing in VS Code. Eslint had issues with the liquid syntax within a JS file. To avoid manually excluding specific files, the logical solution was to segregate liquid and JS.

Overview:

The main JS files in question are located in asset > js and include wins.js, toolkit.js, project.js, hamburger-nav.js, current-projects.js, and current-projects-check.js, totaling six files.
The task was to refactor a JS file using liquid into two distinct files. I successfully transformed a file to ensure its functionality.

Implementation Plan:

Initially, I addressed this by introducing a global variable. While eslint didn't object to the absence of liquid syntax, it wasn't a best practice. Eslint still raised issues since I was referencing uninitialized variables. Additionally, eslint doesn't support front matter, so I ensured no exposure of front matter or template syntax. The goal was to avoid global variables and maintain best practices.

Post file separation, I locally installed eslint to verify the efficacy of the separation in preparation for our eslint adoption. I tested eslint with a preset of enlist:recommended to gauge its utility in detecting JS file issues. I also cleaned up the entire JS file, noting its ability to identify space/tab inconsistencies, unused variables, and potentially fragile code.

While working with eslint, I also integrated htmlhint, an HTML linter, to pre-scan all HTML files in the project. I've observed some actionable issues based on this and have compiled a list, which I'll share via a Google link.

Acknowledgments: 🙇‍♂️

A special thank you to @roslynwythe for patiently addressing all my queries.
Kudos to @Tomomi-K1 for assisting in bug testing and collaboratively researching liquid with me.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

Visuals after changes are applied

github-actions · 2023-08-19T09:34:59Z

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 ronaldpaek-refactor-liquid-js-1442 gh-pages
git pull https://github.com/ronaldpaek/website.git refactor-liquid-js-1442

assets/js/wins.js

+ const overlayOverview = document.querySelector('#overlay-overview');
+ overlayOverview.innerHTML = data[i][overview];
+
+ insertIcons('#overlay-info', data[i][win], 'overlay')


assets/js/wins.js

+ `https://avatars1.githubusercontent.com/u/${ghId}?v=4` :
+ AVATAR_DEFAULT_PATH;
+
+ cloneCardTemplate.querySelector('.wins-card-profile-img').src = profileImgSrc;


assets/js/wins.js

+ let teamInURL = filterParams.team.split(',');
+ let roleIntersection = rolesInCard.filter(x => roleInURL.includes(x));
+ let teamIntersection = teamsInCard.filter(x => teamInURL.includes(x));
+ ((roleIntersection.length == 0) || (teamIntersection.length == 0)) ? card.style.display = 'none' : card.style.display = 'flex'


assets/js/wins.js

+
+
+function hideOverlay(e) {
+ window.event || e.key == 'Escape';


assets/js/wins.js

+
+
+function hideOverlay(e) {
+ window.event || e.key == 'Escape';


ronaldpaek · 2023-08-19T10:08:46Z

I tried to not changed the previous code as much as possible, but I did feel like there were a lot of unused variables and deprecated things, I didn't want my issue and commit to be about that, but CodeQL be bringing these things up loud and clear 😅

mademarc · 2023-08-25T01:56:54Z

Review ETA: 8/24/2023
Availability: 7PM

mademarc

Hey @ronaldpaek the changes on the assets/js/wins.js file is done well.

ronaldpaek · 2023-08-25T05:42:29Z

Hey @ronaldpaek the changes on the assets/js/wins.js file is done well.

Thank you @mademarc

jaasonw · 2023-10-02T02:54:12Z

Review ETA: 10/1/2023
Availability: 12PM-6PM

jaasonw

Functionally, it looks good on my end. I've some random nitpicks that I'm not sure should be in the scope of this PR as much of it isn't your code

jaasonw · 2023-10-02T03:15:10Z

pages/wins/wins.html


- {% include wins-page/wins-filter-template.html %}
- {% include wins-page/wins-card-template.html %}


nit: why did the indentation get changed here (as well as everywhere else)?

jaasonw · 2023-10-02T03:20:53Z

assets/js/wins.js

+ const teamArr = [];
+ const responses = document.querySelector("#responses");
+ responses.querySelectorAll('.wins-card-team:not([style*="display:none"]):not([style*="display: none"]').forEach(item => {
+ let value = item.textContent.replace("Team(s):", "").trim();


nit: a lot of these lets don't actually need to be lets because they're never reassigned and should be const instead. The eslint rule for this is prefer-const

jaasonw · 2023-10-02T03:24:33Z

assets/js/wins.js

+ const roleDropDown = document.getElementById("role-dropdown");
+ const teamDropDown = document.getElementById("team-dropdown");
+
+ for(const [key] of Object.entries(roleHash)) {


nit: using Object.keys() avoids having to destructure an array like this

jaasonw · 2023-10-02T03:29:11Z

assets/js/wins.js

+ teamsInCard = teamsInCard.split(",").map(x => x.trim());
+ let rolesInCard = (card.querySelector('.wins-card-role').textContent.replace("Role(s):", "")).trim();
+ rolesInCard = rolesInCard.split(",").map(x => x.trim());
+ // let cardUnion = [...rolesInCard, ...teamsInCard];


nit: commented code could be removed

jaasonw · 2023-10-02T03:30:34Z

assets/js/wins.js

+ const LINKEDIN_ICON = '/assets/images/wins-page/icon-linkedin-small.svg'
+ const winsCardContainer = document.querySelector('#responses');
+ cards.forEach((card, index) => {
+ // if (card[display] != true) return;


nit: commented code could be removed

jaasonw · 2023-10-02T03:44:01Z

assets/js/wins.js

+ cloneCardTemplate.querySelector('.wins-card-github-icon').setAttribute('hidden', 'true')
+ }
+
+ cloneCardTemplate.querySelector('.project-inner.wins-card-team').innerHTML = `<span class="wins-team-role-color">Team(s): </span> ${card[team]}`;


Using innerHTML like this is a potential XSS vulnerability here if the data source is compromised or not properly sanitized

jaasonw · 2023-10-02T03:45:09Z

assets/js/wins.js

+ }
+
+ const overlayName = document.querySelector('#overlay-name');
+ overlayName.innerHTML = data[i][name];


Similar to my other comment about XSS

ExperimentsInHonesty · 2023-10-24T01:53:40Z

I made a change to your text above to remove fixes, since we don't want it to close the related issue

ronaldpaek added 2 commits August 19, 2023 02:07

Update wins.html file to use liquid in this file

d3a08f2

Seperate liquid logic from js file

e829c27

HackforLABot added this to PR Needs review (Automated Column, do not place items here manually) in Project Board Aug 19, 2023

github-actions bot added role: back end/devOps Tasks for back-end developers Complexity: Large Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly size: 2pt Can be done in 7-12 hours labels Aug 19, 2023

github-advanced-security bot found potential problems Aug 19, 2023

View reviewed changes

ronaldpaek requested review from ExperimentsInHonesty and roslynwythe August 19, 2023 10:09

Refactor code to remove nodes after parsing data

85a1d00

roslynwythe added the Draft Issue is still in the process of being created label Aug 20, 2023

t-will-gillis requested a review from mademarc August 23, 2023 02:27

mademarc approved these changes Aug 25, 2023

View reviewed changes

Project Board automation moved this from PR Needs review (Automated Column, do not place items here manually) to test-approved-by-reviewer (Automated Column, do not place items here manually) Aug 25, 2023

jaasonw self-requested a review October 2, 2023 02:54

jaasonw suggested changes Oct 2, 2023

View reviewed changes

Project Board automation moved this from test-approved-by-reviewer (Automated Column, do not place items here manually) to test-pending-approval (Automated Column, do not place items here manually) Oct 2, 2023

jaasonw mentioned this pull request Oct 3, 2023

ER: Potential XSS Vulnerability in wins.js #5654

Open

5 tasks

roslynwythe mentioned this pull request Feb 29, 2024

Epic: Enable code scanning on JS files #6378

Open

5 tasks

roslynwythe mentioned this pull request Mar 15, 2024

Epic: Manage CodeQL deployment #5005

Open

20 tasks

roslynwythe mentioned this pull request May 6, 2024

ER: Adopting a coding style standard/guideline #6807

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wins.html file to use liquid in this file #5258

Update wins.html file to use liquid in this file #5258

ronaldpaek commented Aug 19, 2023 •

edited by ExperimentsInHonesty

github-actions bot commented Aug 19, 2023

ronaldpaek commented Aug 19, 2023

mademarc commented Aug 25, 2023

mademarc left a comment

ronaldpaek commented Aug 25, 2023

jaasonw commented Oct 2, 2023

jaasonw left a comment •

edited

jaasonw Oct 2, 2023

jaasonw Oct 2, 2023 •

edited

jaasonw Oct 2, 2023

jaasonw Oct 2, 2023

jaasonw Oct 2, 2023

jaasonw Oct 2, 2023

jaasonw Oct 2, 2023

ExperimentsInHonesty commented Oct 24, 2023


		{% include wins-page/wins-filter-template.html %}
		{% include wins-page/wins-card-template.html %}

Update wins.html file to use liquid in this file #5258

Are you sure you want to change the base?

Update wins.html file to use liquid in this file #5258

Conversation

ronaldpaek commented Aug 19, 2023 • edited by ExperimentsInHonesty

Changes Made:

Reason for Changes:

Overview:

Implementation Plan:

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

github-actions bot commented Aug 19, 2023

ronaldpaek commented Aug 19, 2023

mademarc commented Aug 25, 2023

mademarc left a comment

Choose a reason for hiding this comment

ronaldpaek commented Aug 25, 2023

jaasonw commented Oct 2, 2023

jaasonw left a comment • edited

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

jaasonw Oct 2, 2023 • edited

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

jaasonw Oct 2, 2023

Choose a reason for hiding this comment

ExperimentsInHonesty commented Oct 24, 2023

ronaldpaek commented Aug 19, 2023 •

edited by ExperimentsInHonesty

jaasonw left a comment •

edited

jaasonw Oct 2, 2023 •

edited