Skip to content

102502047 #41

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
jack99541008 wants to merge 2 commits into
base: homework
Choose a base branch
from
Open

102502047 #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6960e8
test
jack99541008 Oct 15, 2015
fa3aa86
hw2
jack99541008 Oct 29, 2015
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
42 changes: 23 additions & 19 deletions homework/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
'use strict';

(function() {
var _wrapper = document.querySelector('#note-content-wrapper');

function start() {
window.addEventListener('note-open', function(event) {
var note = event.detail;
resetWrapper();
drawNote(note);
});
(function(exports) {
var ContentManager = function() {
this.wrapper = document.querySelector('#note-content-wrapper');
}

ContentManager.prototype = {
start() {
window.addEventListener('note-open', (function(event) {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

It is better to bind this as the listener, rather than an anonymous function.

var note = event.detail;
this.resetWrapper();
this.drawNote(note);
}).bind(this));
},

function resetWrapper() {
_wrapper.innerHTML = '';
}
resetWrapper() {
this.wrapper.innerHTML = '';
},

function drawNote(note) {
drawNote(note) {
var title = note.title;
var h = document.createElement('h2');
h.textContent = title;
Expand All @@ -27,11 +30,12 @@
p.textContent = passage;
buff.appendChild(p);
});
_wrapper.appendChild(h);
_wrapper.appendChild(buff);
this.wrapper.appendChild(h);
this.wrapper.appendChild(buff);
}

document.addEventListener('DOMContentLoaded', function(event) {
};
/* document.addEventListener('DOMContentLoaded', function(event) {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

You can just remove unnecessary code.

start();
});
})();
});*/
exports.ContentManager = ContentManager;
})(window);
1 change: 1 addition & 0 deletions homework/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<head>
<meta charset="UTF-8">
<title> Homework - Note List </title>
<script src="main.js"></script>
<script defer src="list.js"></script>
<script defer src="content.js"></script>
<link rel="stylesheet" type="text/css" href="main.css" />
Expand Down
92 changes: 51 additions & 41 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,51 @@
'use strict';

(function() {
(function(exports) {

var _listNoteContent = [];
var _wrapper = document.querySelector('#note-list-wrapper');
var ListManager = function() {
this._listNoteContent = [];
this._wrapper =document.querySelector('#note-list-wrapper');
};

function start() {
fetchList(function(data) {
updateList(data);
drawList();
preloadFirstNote();
});
window.addEventListener('click', function(event) {
onNoteOpen(event);
});
}
ListManager.prototype = {

start() {
this.fetchList().then((function(response) {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

nit: I recommend to use Arrow function rather than just function binding, although they're both legit.
See:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Lexical_this

this.updateList(response);
this.drawList();
this.preloadFirstNote();
}).bind(this)).catch((function() {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

Serious issue: don't catch error without logging AND re-throwing it, since this will make the error disappear silently. Consider what will happen if the chain contains an error but your program won't alert anything about that.

}).bind(this));


window.addEventListener('click', (function(event) {
this.onNoteOpen(event);
}).bind(this));
},

function onNoteOpen(event) {
onNoteOpen(event) {
if (event.target.classList.contains('note-title')) {
var id = event.target.dataset.noteId;
var content = _listNoteContent[id];
var content = this._listNoteContent[id];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
};
}
},

function preloadFirstNote() {
if (_listNoteContent.length !== 0) {
var content = _listNoteContent[0];
preloadFirstNote() {
if (this._listNoteContent.length !== 0) {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

nit: It's better to put the condition outside a function wholly wrapped by only one branch.

var content = this._listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
}
}
},

function updateList(list) {
_listNoteContent = list;
}
updateList(list) {
this._listNoteContent = list;
},

function drawList() {
var list = _listNoteContent;
drawList() {
var list = this._listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
var buff = document.createDocumentFragment();
Expand All @@ -52,28 +59,31 @@
buff.appendChild(li);
});
ul.appendChild(buff);
_wrapper.appendChild(ul);
}
this._wrapper.appendChild(ul);
},

function fetchList(afterFetch) {
var xhr = new XMLHttpRequest();
xhr.open('GET', 'http://127.0.0.1:8000/demo-list-notes.json', true);
xhr.responseType = 'json';
xhr.onreadystatechange = function(e) {
// Watch out: we have a mysterious unknown 'this'.
if (this.readyState === 4 && this.status === 200) {
fetchList() {
return new Promise((function(resolve, reject) {
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

Good. You have successfully "Promisified" this method.

var xhr = new XMLHttpRequest();
xhr.open('GET', 'http://127.0.0.1:8000/demo-list-notes.json', true);
xhr.responseType = 'json';
xhr.onreadystatechange = function(e) {
// Watch out: we have a mysterious unknown 'this'.
if (this.readyState === 4 && this.status === 200) {
var listData = this.response;
// The flow ends here.
afterFetch(listData);
} else if (this.status !== 200 ){
resolve(listData);
} else if (this.status !== 200 ){
reject('FETCHING FAILED: ' + this.status + ' ' + this.readyState);
// Ignore error in this case.
}
}
};
xhr.send();
}).bind(this));
}
};

exports.ListManager = ListManager;
})(window);

document.addEventListener('DOMContentLoaded', function(event) {
start();
});

})();
9 changes: 9 additions & 0 deletions homework/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

// Kick off
document.addEventListener('DOMContentLoaded', function(event) {
var listManager = new ListManager();
var contentManager = new ContentManager();
listManager.start();
contentManager.start();
});
1 change: 1 addition & 0 deletions homework/test/test-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ describe('Test > ', function() {

it('will test some pure functions', function() {
// Write any pure function assertion here.
assert.equal(1+2, 3);
Copy link
Contributor

@snowmantw snowmantw Nov 8, 2015

Choose a reason for hiding this comment

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

No. You need to create a pure method in one manager, and then test to invoke it here.

});
});