Refactor java script so that project meetings page sources meeting data from vrms data.json #6059 #7873
Conversation
…functions like in right-col-content.js
…oject-Meetings-page-sources-meeting-data-from-vrms_data.json-hackforla#6059
…roject-meetings.js file
|
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. |
github-actions
bot
added
role: front end
|
Review ETA: EOD 2/08/25 |
There was a problem hiding this comment.
The code refactor is really good and meets the condition of the case. Looking at both live and this branch changes, there is a reduce in latency. Good job. The only changes I request are just simplifying the logic so it's easier to read and debug. Will approve after.
|
|
||
| const formatedEvents = formatEventData(eventData) | ||
|
|
||
| for (const [key, value] of Object.entries(formatedEvents)) { |
There was a problem hiding this comment.
I would recommend changing [key, value] to [day, event] for readability
| if (!value.length) { | ||
| placeToInsert.insertAdjacentHTML( | ||
| "beforeend", | ||
| `<li>There are no meetings scheduled.</li>` | ||
| ); | ||
| } else { | ||
| value.forEach((event) => { | ||
| if (event) { | ||
| // If a project doesn't have an hflaWebsiteUrl, redirect to the project's Github page | ||
| if (event.hflaWebsiteUrl == "") { | ||
| event.hflaWebsiteUrl = event.githubUrl | ||
| } | ||
| let eventHtml; | ||
| // insert the correct html for the current page | ||
| if (page === "events") { | ||
| eventHtml = `<li>${event.start} - ${event.end} </li><li><a href="${event.hflaWebsiteUrl}">${event.name}</a> ${event.meetingName}${event.dsc.length > 0 ? "*" : ""}</li>`; | ||
| } else { | ||
| if (event.dsc != "") event.meetingName += ", "; | ||
| eventHtml = `<li>${event.start} - ${event.end} <a href="${event.hflaWebsiteUrl}">${event.name}</a> ${event.meetingName} ${event.dsc}</li>`; | ||
| } | ||
| placeToInsert.insertAdjacentHTML("beforeend", eventHtml); | ||
| } |
There was a problem hiding this comment.
An if/else inside an if/else can usually be simplified. It just makes it easier for readability and not having to keep track of different types of branch logic. I'll attach the code with fewer if/else:
// check if the day has any events
if (!events.length) {
placeToInsert.insertAdjacentHTML(
"beforeend",
`<li>There are no meetings scheduled.</li>`
);
continue;
}
events.forEach((event) => {
if(!event) return;
// If a project doesn't have an hflaWebsiteUrl, redirect to the project's Github page
if (!event.hflaWebsiteUrl) event.hflaWebsiteUrl = event.githubUrl
let eventHtml;
// insert the correct html for the current page
if (page === "events") {
eventHtml = `<li>${event.start} - ${event.end} </li><li><a href="${event.hflaWebsiteUrl}">${event.name}</a> ${event.meetingName}${event.dsc.length > 0 ? "*" : ""}</li>`;
} else {
if (event.dsc != "") event.meetingName += ", ";
eventHtml = `<li>${event.start} - ${event.end} <a href="${event.hflaWebsiteUrl}">${event.name}</a> ${event.meetingName} ${event.dsc}</li>`;
}
placeToInsert.insertAdjacentHTML("beforeend", eventHtml);
});
| }; | ||
| responseData.forEach((item) => { | ||
| let day_of_week = getDayString(item.date); | ||
| return_obj[day_of_week].push(display_object(item)); |
There was a problem hiding this comment.
From what I understand with js, even if a function has no return value, it'll return an undefined. We can avoid the unnecessary push by declaring it and adding conditional here and readjusting the display_object if conditional to be null if met.
const event = display_object(item);
if (event) return_obj[day_of_week].push(event);
for display_object:
if (!item?.project?.name || item.project.name === "Hack4LA" || /^Test\b/i.test(item.project.name)) {
return null;
}
Hi @Cloid , The recommended changes do make the code a lot easier and simpler. However, I see scope for further refactoring the almost similar |
|
I agree that consolidating it into a single utility file would be more efficient. I was working under the assumption that this wasn’t an option. If that is the end goal, it would be helpful to mention it in both the PR and the Issue itself to ensure clear communication with the team. I'll approve it right after. Definitely, good catch on the similarities. |
|
Review ETA: 12 Feb 2025 @ 6 PM (GMT) |
There was a problem hiding this comment.
Hey @innith, thank you for working on this. Nice work!
Kudos
- The pull request is made with the correct branch.
- The pull request post contains a linked issue, and what changes you made and why are clear.
- The changes to the files are accurate. (Even though the changes that are made to the
assets/js/project-meetings.jsare copied from lines 26 - 123 ofassets/js/right-col-content.jsit is ok to do so in this particular case.) - You have claimed the orginiating issue well (Refactor JavaScript so that Project Meetings page sources meeting data from vrms_data.json #6059).
- I can see the changes in my local environment.
Suggested changes
- For the "CodeQL Alerts" section of the PR post, please check off the relevant box(es).
- For the Screenshots section of the PR post, you can remove the dropdowns and put "No visual changes to website".
- You could also update the PR post title like this: Refactor JavaScript so that Project Meetings page sources meeting data from vrms_data.json
After making these changes, please re-request a review from me. Feel free to reach out if you need help with anything.
Fixes #6059
What changes did you make?
assets/js/project-meetings.jsto source data fromdata/external/vrms_data.json, using functions inassets/js/utility/vrms-events.jsassets/js/project-meetings-check.jsto evaluate the changes made toassets/js/project-meetings.jsWhy did you make the changes (we will use this info to test)?
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)
Visuals before changes are applied
No visual changes applied
Visuals after changes are applied
No visual changes applied