Release 0.2: Pull Request #4
This weeks PR was on the sButtons repo which was a website that provided many different kinds of buttons that developers could use on there next project.
For my last PR I wanted to fix a bug fix that wasn't immediately obvious. In this case it was a website throwing a jQuery error within the console which stopped some scripts from working correctly.
To start I could clearly see which file was involved with this error which was the index.js file so I started to investigate were the property 'left' was used. The undefined property was located within the isOnScreen() function which job it was to gather the current screen window data. At first I thought the error was within in this function but after looking up this error I found out it was most likely caused by undefined html element provided to the function.
Moving on from there I looked at every place the function was called within index.js. Within the checkSidebar() function it was called twice, so now I started to look at the logic for this function.
There were two places in the program where checkSidebar() was called. Once when the page refreshes and the other within a function that is called when the user scrolls down the page. Commenting out these function calls and running the code no longer showed the error so I now narrowed it down to this function.
As seen with the error screen above, you can see "HTMLDocument.<anonymous>". This is actually one of the calls to the checkSidebar() function. Here the function is called with whatever the current html element is, the problem with this is that the sites pages have different html elements and the checkSidebar() function was looking for certain elements. The function was looking for both "main-head" and "footer". Each page had a "footer" class, but not every page had a "main-head" class which is the reason why this error only happened on the main page.
To fix this I added a if statement encasing the code for "main-head" to check if it even exists before the code would try to call isOnScreen() with that element. To check If the element exists you can use ".length" on a jQuery element and check its return value to see if it exists on the current page.
if ($(".main-head").length) {
if (!$(".main-head").isOnScreen()) {
$(".sidebar").addClass("scrolling");
} else {
$(".sidebar").removeClass("scrolling");
}
}
After confirming each page on the site was still working properly and no errors where shown within the console log I could say now that the issue was fixed so I made a pull request following there guidelines.
Maintainer Interactions
This PR had the most maintainer feedback/suggestions out of all four PR combined. At first I was asked to removed an extra space I left in the code, which I fixed quickly. After this another maintainer asked if I could implement this fix onto the second half of the function which was the "footer" class. I then implemented the changed and pushed the new code up.
Here's the full function I worked with after my changes:
// Navbar toggle here
function checkSidebar() {
if ($(".main-head").length) {
if (!$(".main-head").isOnScreen()) {
$(".sidebar").addClass("scrolling");
} else {
$(".sidebar").removeClass("scrolling");
}
}
if ($(".footer").length) {
if (!$(".footer").isOnScreen()) {
$(".sidebar").removeClass("height-shift");
} else {
$(".sidebar").addClass("height-shift");
}
}
}
After this my code was merged into the master branch with no conflicts and they even made a new PR to have me added to the contributions list on their site, which was pretty cool.
Overall this PR was really nice to work with since they responded quickly and they were super nice about my work.
What did I learn?
This PR's focus for me was to dig through code and understand how it all connected to solve a bug. I would say I'm better now at understanding and how to work through larger projects.
Comments
Post a Comment