Issue
The second issue I worked on is for a project called CDNJS, a website hosting all kinds of popular libraries for JavaScript, CSS, SWF, images, and etc. There is a search feature on the main page that enables users to search any libraries they want. It returns matching libraries in a table view containing library names, descriptions, links, and other details. To go into the detailed library page, users have to click on the library name specifically from the table. An issue is raised here pointing that this is a tedious job, especially if the name is short because there are really short names too such as ‘d3’. The issue was requesting to make the library name and description a single button to make selection easier.
Working Process
As I mentioned in the previous post that I want to have more communications for the next issue, I was able to go through more communications this time. First I started by asking if I could work on the issue. One of the members kindly responded providing the relevant code. I read through the HTML and JavaScript files to investigate the code. The code was designed dynamically so that when the user starts typing in the search box, it passes the JavaScript code generating a table with all matching library information.
'<p>
<a itemprop="name" href="/libraries/' + hit.name + '">' +
hit._highlightResult.name.value + '</a>
</p>' +
'<p class="text-muted">' + description + '</p>' +This is how the original code looks like in the relevant part. Only the library name is passed as <a> link so that it’s clickable. The first method I came up with is surrounding both the library name and description in a <div>, and making it clickable using jQuery because the website was using a lot of jQuery methods.
// html
'<div class="clickable" style="cursor:pointer;">' +
'<p>
<a itemprop="name" href="/libraries/' + hit.name + '">' +
hit._highlightResult.name.value + '</a>
</p>' +
'<p class="text-muted">' + description + '</p>' +
'</div>' +
// jQuery
$(document).on('click', '.clickable', function() {
window.location = $(this).find("a").attr("href");
})Once the <div> area surrounding the name and description is clicked, the jQuery function is executed to set the current position of the page to the position pointed by the ‘href’ attribute of the <a> link. I made a PR with this change and it passed all the automated tests. But the reviewer didn’t like the change and wanted to make it simpler by just moving the <a> element instead of adding another jQuery function.
'<a style="text-decoration: none;" itemprop="name" href="/libraries/' + hit.name + '">' +
hit._highlightResult.name.value +
'<p class="text-muted">' + description + '</p>' +
'</a>' +This is the second solution I came up with. I removed the <div> element and the jQuery function and moved <a> element to surround the description part as well. But I noticed that this solution makes the whole description part underlined as well which looks a bit messy. So I added inline style to remove ‘text-decoration’. I pushed the new commit to the PR, and the reviewer left a few more points that need to be changed based on project standards.
Now I need to make more changes according to the review comments and hope that it can be merged after that.
Overall
Since I didn’t have any communication involved in my first PR, I enjoyed the experience having communication with the reviewer. As I received feedbacks, fixed the code according to the comments, and resubmitted the PR, I felt I was actually working at a company. I believe that all these processes will improve my programming and communication skills.
Minuk's Blog 