Skip to content

101502014 homework #32

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

101502014 homework #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
628dd62
Bind this in Event handlers
timmycheng1221 Oct 26, 2015
7ea84a2
Deal with object properties w/ constructor & prototype
timmycheng1221 Oct 26, 2015
bea1d3a
Deal with asynchronous flows with Promise
timmycheng1221 Oct 26, 2015
7dbccee
Write my first test case for a pure function
timmycheng1221 Oct 26, 2015
e236862
debug
timmycheng1221 Oct 27, 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
66 changes: 36 additions & 30 deletions homework/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,43 @@
'use strict';

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

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

NoteContentManager.prototype = {

function resetWrapper() {
_wrapper.innerHTML = '';
}
start() {
window.addEventListener('note-open', (function(event) {
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

Instead of binding an anonymous function, you should set the listener argument as this, and deal with the event in the handleEvent method.

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

function drawNote(note) {
var title = note.title;
var h = document.createElement('h2');
h.textContent = title;
var passages = note.passages;
var buff = document.createDocumentFragment();
passages.forEach(function(passage) {
var p = document.createElement('p');
p.classList.add('note-passage');
p.textContent = passage;
buff.appendChild(p);
});
_wrapper.appendChild(h);
_wrapper.appendChild(buff);
}
resetWrapper() {
this._wrapper.innerHTML = '';
},

drawNote(note) {
var title = note.title;
var h = document.createElement('h2');
h.textContent = title;
var passages = note.passages;
var buff = document.createDocumentFragment();
passages.forEach(function(passage) {
var p = document.createElement('p');
p.classList.add('note-passage');
p.textContent = passage;
buff.appendChild(p);
});
this._wrapper.appendChild(h);
this._wrapper.appendChild(buff);
}

};

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

})(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 @@ -5,6 +5,7 @@
<title> Homework - Note List </title>
<script defer src="list.js"></script>
<script defer src="content.js"></script>
<script defer src="main.js"></script>
<link rel="stylesheet" type="text/css" href="main.css" />
</head>
<body>
Expand Down
142 changes: 75 additions & 67 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,79 +1,87 @@
'use strict';

(function() {
(function(exports) {

var _listNoteContent = [];
var _wrapper = document.querySelector('#note-list-wrapper');
var NoteListManager = 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);
});
}
NoteListManager.prototype = {

function onNoteOpen(event) {
if (event.target.classList.contains('note-title')) {
var id = event.target.dataset.noteId;
var content = _listNoteContent[id];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
};
}
start() {
this.fetchList()
.then((function(data) {
this.updateList(data);
this.drawList();
this.preloadFirstNote();
}).bind(this)).catch((function(){
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

Serious issue: you need to catch it, which is correct. But the point is you need to log AND re-throw the error in the catch body. To log it is to prevent the following chain will intercept the error and show nothing in your console, which could devastate your debugging strategy wholly. To re-throw it is to make sure the next chain will be interrupted to prevent unexpected behaviors.

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

function preloadFirstNote() {
if (_listNoteContent.length !== 0) {
var content = _listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
}
}
onNoteOpen(event) {
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

It's a convention to prefix on to the method handle the corresponding event. So it should be renamed as onNoteClicked.

if (event.target.classList.contains('note-title')) {
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

If a function body is wrapped by a conditional statement with only one branch, the if...else should be moved into the caller, rather than the callee.

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

function updateList(list) {
_listNoteContent = list;
}
preloadFirstNote() {
if (this._listNoteContent.length !== 0) {
var content = this._listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
}
},

function drawList() {
var list = _listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
var buff = document.createDocumentFragment();
list.forEach(function(note, i) {
var li = document.createElement('li');
li.dataset.noteId = i;
li.classList.add('note-title');
li.textContent = note.title;
// Note: buff is captured, so we now have a
// little closure naturally.
buff.appendChild(li);
});
ul.appendChild(buff);
_wrapper.appendChild(ul);
}
updateList(list) {
this._listNoteContent = list;
},

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) {
var listData = this.response;
// The flow ends here.
afterFetch(listData);
} else if (this.status !== 200 ){
// Ignore error in this case.
}
};
xhr.send();
drawList() {
var list = this._listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
var buff = document.createDocumentFragment();
list.forEach(function(note, i) {
var li = document.createElement('li');
li.dataset.noteId = i;
li.classList.add('note-title');
li.textContent = note.title;
// Note: buff is captured, so we now have a
// little closure naturally.
buff.appendChild(li);
});
ul.appendChild(buff);
this._wrapper.appendChild(ul);
},

fetchList() {
return new Promise((function(resolve , reject) {
Copy link
Contributor

@snowmantw snowmantw Nov 10, 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" it.

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.
resolve(listData);
} else if (this.status !== 200 ){
// Ignore error in this case.
reject(xhr);
}
};
xhr.send();
}).bind(this));
}
}

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

})();
})(window);
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 noteListManager = new NoteListManager();
var noteContentManager = new NoteContentManager();
noteListManager.start();
noteContentManager.start();
});
22 changes: 18 additions & 4 deletions homework/test/test-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
describe('Test > ', function() {
describe('Test NoteListManager ', function() {

beforeEach(function() {
test = new NoteListManager();
});

it('will test some pure functions', function() {
// Write any pure function assertion here.
});
it('will test updateList functions', function() {
// Write any pure function assertion here.{
var dummyList = [
{ "title": "Indian 'mystery woman' Geeta says family not hers",
"passages": [
"An Indian woman who was stranded in Pakistan for a decade has returned home, but says the family she had identified in photos is not hers.",
"India's Foreign Minister Sushma Swaraj said the woman, named Geeta, is refusing to recognise her family.",
"Geeta arrived in Delhi on Monday morning, days after she identified her family in photos sent from India."
]}
];

test.updateList(dummuyList);

assert.equal(test._listNoteContent, dummuyList);
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

It's good to see you test a real method rather than just a pure function.

});
});