Week3 #78
Week3 #78
Conversation
remarcmij
left a comment
remarcmij
left a comment
There was a problem hiding this comment.
Hi Mehmet, this is a pretty good job. Everything works, and your app is responsive. But it is not ARIA-compliant and there are some other improvement points. However, there is nothing to stop me from approving your PR and wishing good luck with the next module.
| onkeydown: `if(event.key === 'Enter'){window.open('${contLink}', '_blank');};`, | ||
| role: 'link', | ||
| tabindex: 0 | ||
| }); |
There was a problem hiding this comment.
Creating a table for this list is not the best solution. For one thing, it is not ARIA-compliant. Unless you add some extra ARIA attribute a screen reader will identify this as a table and not as a list. You also need to do complicated things here to respond to mouse click and the keyboard. If instead you used a ul with li and embedded a tags all this would become simpler. This is what another student did in terms of HTML structure:
<ul class="contributor-list">
<li class="contributor-item">
<a target="_blank" href="https://github.com/gijscor">
<img class="contributor-avatar" src="https://avatars0.githubusercontent.com/u/15912395?v=4">
<div class="contributor-data">
<div class="contributor_name">gijscor</div>
<div class="contributor_badge">12</div>
</div>
</a>
</li>
<li class="contributor-item">
<a target="_blank" href="https://github.com/wouterkleijn">
<img class="contributor-avatar" src="https://avatars1.githubusercontent.com/u/37488847?v=4">
<div class="contributor-data">
<div class="contributor_name">wouterkleijn</div>
<div class="contributor_badge">4</div>
</div>
</a>
</li>
</ul>There was a problem hiding this comment.
Actually using ul-li elements would be simpler than the table element. However, I added some extra codes, therefore a user can navigate through all interactive elements using the keyboard (using the Tab and Enter keys). Pressing Enter on such an element should be equivalent to clicking the mouse.
I did not add some aria codes about screen reader. Instead of replacing the table element by ul-li elements, is it enough if I add some aria codes about screen reader?
There was a problem hiding this comment.
I think you will find that anchor elements (a) are navigable with the keyboard (Enter, Tab). In general, if you use HTML elements that are appropriate for the semantic context you are almost guaranteed to be ARIA-compliant. Only when you deviate from that you need extra ARIA attributes on your elements to provide the semantic context. So, when you want to create a list, use ul or ol. If you use a table than you would need add ARIA roles to label the table as a list and each row as a list item.
Check section 2 of the link below (taken from the HTML/CSS module) for some simple rules for ARIA-compliance.
| alt: 'loading gif', | ||
| class: 'loadingGif' | ||
| }); | ||
| xhr.onreadystatechange = () => { |
There was a problem hiding this comment.
This looks like a nice gimmick but really only makes sense if, on average, you expect a response to take considerable time. In this particular case the response comes so quickly (< 200ms) that the GIF only produces an incomprehensible flicker. So definitely use this in other cases where the response takes time, but in this case I would leave it out.
Also, as a general principle, you should try and avoid mixing 'concerns' in a single function. The fetchJSON() that deals with fetching data should not also be concerned with manipulating the DOM.
There was a problem hiding this comment.
Thank you @remarcmij , I added a loading status, because Jawhar wanted me to add a loading status as a challenge. Therefore I understood better how readystate works.
I can change the place of the loading status codes or I can remove all of it. Also I can add a setTimeout method, therefore the loading status can become more comprehensible.
| this.Description = sourceObj.description; | ||
| this.Forks = sourceObj.forks; | ||
| const dateOfUpdate = new Date(sourceObj.updated_at); | ||
| this.Updated = dateOfUpdate.toLocaleString(); |
There was a problem hiding this comment.
Object property names should be in camelCase.
There was a problem hiding this comment.
Actually, you are right, I named them with starting capital letters because I used them in the web page. Here is the example: Forks: 5
| } else { | ||
| createAndAppend('pre', root, { html: JSON.stringify(data, null, 2) }); | ||
| } | ||
| //this is an object creator |
There was a problem hiding this comment.
Technically, this is called a constructor function.
|
|
||
| container.innerHTML = ""; | ||
|
|
||
| const info = new RepositoryInfo(repoData); |
There was a problem hiding this comment.
I'm not sure whether creating on object here rather than using the repo data directly adds value. Perhaps this was just a finger exercise?
|
Thank you @remarcmij , I will make changes on my code in the way you lead. Thank you for everything you taught. :) |
3 participants
No description provided.