-
-
Notifications
You must be signed in to change notification settings - Fork 777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor HTML and JavaScript so Projects and Projects-Check page share same files #5978
Refactor HTML and JavaScript so Projects and Projects-Check page share same files #5978
Conversation
- Create Tools section in tdm-calculator.md and transfer Figma - Move Figma and Miro to Tools section and remove Technologies section in youthjusticenav.md
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.
Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:
|
Availability: Evenings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there @jphamtv nice work with the issue all the work assigned to you for refactoring is done properly, the changes are made in the html file as well as the current-project.js
are apt and looks good. The to/from branch is correct and so are the visual changes as per specification in the issue.
Also thanks for giving extra effort in pointing out the filter discrepancies, and changing it out with a correct solution.
I approve this great work there.....
eta: 12/27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jphamtv thank you for tackling this issue. The "Projects-Check" page is including the current-projects.html
but is displaying the filter options for Technologies, Languages, and Tools- and the filter results appear to be correct in Docker.***
***'Ruby' and 'Svelte' are correctly appearing on the Languages filter. This is out of the scope of this PR but I am wondering why these two do not appear on the "Projects" page filter for Languages / Technologies?
(continues)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @freaky4wrld - good call for making the edits to tdm-calculator.md
and youthjusticenav.md
and for flagging them for review.
I do have a comment about lines 641 and 642: on 641, "data-technologies" is defined to combine technologies and languages (to match the filter on the "Projects" page), and on 642 the language data is included a second time in "data-languages". I don't believe this affects the visuals or functionality for either the "Projects" or the "Projects Check" pages. Do you have any thoughts about this?
Fantastic job!
I will mark this as a "Comment" for right now.
Hey @t-will-gillis , thanks for your comments. I think I know why this is happening and will test it out.
|
@t-will-gillis - great catch regarding the filter's exclusion of Ruby & Svelte. The issue seemed to stem from the conditional at line 212, (
Regarding the data-languages data: it's included specifically for the "Projects Check" page. Since this page's filter separately categorizes technologies and languages, we require distinct datasets for each. This setup is necessary for the way the filter list and frequency count are generated based on the original code structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jphamtv thanks for the reply and checking out why Ruby & Svelte aren't on the "Projects" page. Since you have already worked out a solution, would you like to create an issue (or an ER + issue) for addressing this bug? Let me know
Regarding the second item, about lines 641 and 642: I probably did not explain myself well here, my apologies. What I was meaning to say is that it is a bit quirky to have two different datasets for the two pages but call both of them 'technologies'. So on line 641 this 'data-technologies' means 'technologies + languages' as used on the "Projects" page, as opposed to a separate 'technologies' and a separate 'languages' as used on the "Projects-Check" page. However, this is outside the scope of your issue, and I don't think the <li>
attributes affect either of the two pages.
Thanks for you great work on this!
Hey @t-will-gillis - Yes, I'll create an issue to address Ruby and Svelte not appearing on the "Projects" page filter for Languages / Technologies. Thank you for the clarification. I agree, the current setup combining 'technologies + languages' did seem quirky. I'm also wondering if it would be better to have separate categories for 'Technologies' and 'Languages' for the Projects page filter. This would better match the Project Cards, where these are already separate categories. Plus, with such a large list of items, a separation similar to the Projects Check page could improve the user experience. Appreciate the review and insights from you and @freaky4wrld on this PR! |
Removed this file as it's scripts were obviated by a previous PR: hackforla#5978
Removed this file as it's scripts were obviated by a previous PR: #5978
Fixes #4960
What changes did you make?
_pages/projects-check.html
:current-projects.html
instead ofcurrent-projects-check.html
assets/js/current-projects.js
:createFilter
function to handle separate technologies, languages, and tools filters on the Projects-Check pageprojectCardComponent
function to accurately update filter item count on the Projects-Check page_projects/tdm-calculator.md
:_projects/youthjusticenav.md
:Why did you make the changes (we will use this info to test)?
_projects/
folder were made to fix discrepancies so the filters function correctly - For solution reference, see the comments in Issue Added Tools Filter to Projects Check Page #5444. Screenshots of the issue are provided in the before section below.Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Before updating HTML and JavaScript files:
After updating HTML and JavaScript files the filters were not functioning correctly due to the circled discrepancies:
Visuals after changes are applied