basic api query example, update readme #1
Conversation
| <p id="basics"></p> | ||
| <br> | ||
| <p id="summary"></p> | ||
| <script> |
There was a problem hiding this comment.
There's nothing wrong with including JavaScript within an HTML file. What I said to avoid was stuff like this:
<a href="#" onclick="console.log('This is some JavaScript');">Click Me</a>There was a problem hiding this comment.
Oh, yeah, inline stuff is awful.
index.html
Outdated
| <br> | ||
| <p id="summary"></p> | ||
| <script> | ||
| (function() { |
There was a problem hiding this comment.
Your code will be executed as soon as it is encountered. Notice the document.getElementById() call. It relies on the fact that the DOM is ready, so by immediately making this call, there's a change that it returns null if the page hasn't be parsed yet. This is fine for your current setup (placing the script at the end of the body), as all the html will be parsed before the JavaScript is reached. However, you'll likely segregate your JS code into their own files in the future, and in doing so, you'll encounter this issue.
Look into this: https://developer.mozilla.org/en-US/docs/Web/Events/DOMContentLoaded
index.html
Outdated
| document.getElementById("searchButton").addEventListener('click', makeQuery); | ||
|
|
||
| function makeQuery() { | ||
| httpRequest = new XMLHttpRequest(); |
There was a problem hiding this comment.
Consider replacing XMLHttpRequest with fetch. On top of being newer, Fetch's syntax is superior and it is generally less of a hassle to work with.
index.html
Outdated
| var term = document.getElementById("query").value; | ||
|
|
||
| if (!httpRequest) { | ||
| alert('Giving up :( Cannot create an XMLHTTP instance'); |
There was a problem hiding this comment.
Please don't use alert(). It provides a poor user experience. Use console.error(string) for now. In the future, we can incorporate errors directly into the page.
There was a problem hiding this comment.
Oh, I think that was a holdover from an example I was looking at. Will do as you suggest.
index.html
Outdated
| var response = JSON.parse(httpRequest.responseText); | ||
| var symbol = response.hits[0].symbol | ||
| var geneId = response.hits[0]._id | ||
| document.getElementById("basics").innerHTML = 'Gene Symbol: ' + symbol + "; Gene ID: " + geneId; |
There was a problem hiding this comment.
Directly modifying an element's innerHTML is very expensive. On top of that, parsing external data as HTML makes you vulnerable to Cross-site Scripting (XSS).
Use textContent instead.
There was a problem hiding this comment.
Ah, good to know. Yet again, poor examples abound.
index.html
Outdated
| if (sumRequest.status === 200) { | ||
| var response = JSON.parse(sumRequest.responseText); | ||
| var summary = response.summary | ||
| document.getElementById("summary").innerHTML = 'Gene Summary: ' + response.summary; |
also catch responses with no hits
|
All expected changes (and a few others) made. I'm going to go ahead and merge, as I don't think it needs another review. Next up will be adding the species filter and fleshing out the information displayed. |
|
http://markdalgleish.com/2011/03/self-executing-anonymous-functions/ More about self-executing functions. I don't think you need to have one here, but it's good practice to properly scope stuff. |
No description provided.