Skip to content

Ex8 #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zyahav wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Ex8 #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
85316b4
Merge pull request #2 from myok12/master
zyahav Dec 2, 2016
01776e1
Merge https://github.com/myok12/practice
zyahav Dec 6, 2016
6d0dd6b
ignoring invalid files
zyahav Dec 14, 2016
1124865
initial DOM
zyahav Dec 14, 2016
5c3b467
all files
zyahav Dec 22, 2016
dcca942
Add requirde
zyahav Dec 22, 2016
05a6233
Delete updatePhoneBook.js
zyahav Dec 22, 2016
b455386
Delete style.css
zyahav Dec 22, 2016
51cd785
Delete ajax.js
zyahav Dec 22, 2016
97ebba7
Delete data.json
zyahav Dec 22, 2016
e971f83
move files to dir
zyahav Dec 22, 2016
8edf9b7
Merge branch 'ex8' of https://github.com/zyahav/practice into ex8
zyahav Dec 22, 2016
81f4de6
edit phonebook.html
zyahav Dec 22, 2016
b091a64
remove lib from root
zyahav Dec 22, 2016
1fc7af5
indent
zyahav Dec 22, 2016
529657f
remove syntax error
zyahav Dec 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
node_modules
*.*~
.*.swp
18 changes: 18 additions & 0 deletions ex8/js/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
function ajax_get(url, callback) {
xmlhttp = new XMLHttpRequest();
Copy link
Owner

@myok12 myok12 Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing var statement.

xmlhttp.onreadystatechange = function() {
if (xmlhttp.readyState == 4 && xmlhttp.status == 200) {
Copy link
Owner

@myok12 myok12 Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen is readyState!==4 or status!==200. Right now, nothing (your callback isn't called), which is probably not good.
You can simulate this by calling ajax_get in the console, or by a click of a button, after you have killed your local HTTP server.

console.log('responseText:' + xmlhttp.responseText);
try {
var data = JSON.parse(xmlhttp.responseText);
Copy link
Owner

@myok12 myok12 Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: while this is fine in javascript, it's customary (and mandatory in other languages) to move the var data outside of the try, because if the JSON.parse throws an error, data will remain undefined.
So you can do this instead:
var data = null; // or undefined; try{ data = JSON.parse(...) } . . . callback(data);

} catch(err) {
console.log(err.message + " in " + xmlhttp.responseText);
return;
}
callback(data);
Copy link
Owner

@myok12 myok12 Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you fine with the callback being called with undefined (if the responseText cannot be JSON-parsed? Maybe throw an error instead (or signal something else to the callback (two standard options: 1) have two callbacks - one for success - with data, one for failure - with some error message) 2) have a first param to the callback be error - if it's null, the next param is the parsed json)

}
};

xmlhttp.open("GET", url, true);
xmlhttp.send();
}
7 changes: 7 additions & 0 deletions ex8/js/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
requirejs.config({
baseUrl: 'js',
paths: {
ajax: 'ajax',
Copy link
Owner

@myok12 myok12 Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use requirejs' paths for other people's code, not yours.
This just alias updatePhoneBook to update. If you want it to be called update, rename the file; if you want it to be called updatePhoneBook, change the require call for it.

update: 'updatePhoneBook'
}
});
Loading