Skip to content

Homework #30

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

Homework #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba02d4f
bind this in event handlers
CaeserNieh Oct 24, 2015
0b600d0
repair
CaeserNieh Oct 24, 2015
0a54cd4
Bind this into event handler
CaeserNieh Oct 24, 2015
119d2e7
Deal with object properties w/ constructor & prototype
CaeserNieh Oct 24, 2015
189c863
Deal with object properties w/ constructor & prototype
CaeserNieh Oct 24, 2015
3e72c65
Deal with object properties w/ constructor & prototype
CaeserNieh Oct 24, 2015
eac88c4
add main.js
CaeserNieh Oct 24, 2015
5105ee3
Deal with asynchronous flows with Promise
CaeserNieh Oct 24, 2015
36ae82c
Write your first test case for a pure function
CaeserNieh Oct 24, 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
64 changes: 35 additions & 29 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 ContentManager = function(){
this._wrapper = document.querySelector('#note-content-wrapper');
}

function resetWrapper() {
_wrapper.innerHTML = '';
}
ContentManager.prototype = {
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 a function as the listener, you should set the argument as this and deal with the event in the handleEvent method.

var note = event.detail;
this.resetWrapper();
this.drawNote(note);
}).bind(this));
},
// original content.js start function
// call original contest.js resetWrapper function use "this"
// following are the same
resetWrapper(){
_wrapper.innerHTML = '';
},

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);
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.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 @@ -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
88 changes: 48 additions & 40 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,53 @@
'use strict';

(function() {
//the way of how to refactor list.js is the same as content.js
//repair some error in list.js
// add promise into it

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

function start() {
fetchList(function(data) {
updateList(data);
drawList();
preloadFirstNote();
});
window.addEventListener('click', function(event) {
onNoteOpen(event);
var ListManager = function(){
this._listNoteContent = [];
this._wrapper = document.querySelector("#note-content-wrapper");
}

ListManager.prototype = {
start() {
this.fetchList().then((function(data) {
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 recommended to use Arrow function instead of calling bind API, since it makes code more concise. See:

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

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 the error in order to log and re-throw it. The reason is logging can prevent the following chains ignore it and make troubles when you're debugging, while re-throw it to make sure the next chain will be interrupted, rather than to perform the actions which may lead to unexpected behaviors.

}).bind(this));

window.addEventListener('click', (function(event) {
this.onNoteOpen(event).bind(this));
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 corresponds to the event name. So in fact it should be renamed as onNoteClicked.

});
}

function onNoteOpen(event) {
if (event.target.classList.contains('note-title')) {
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 }));
};
}
};
}

function preloadFirstNote() {
if (_listNoteContent.length !== 0) {
var content = _listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
preloadFirstNote() {
if (_listNoteContent.length !== 0) {
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, it's better to move the if...else into the caller, rather than the callee.

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

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

function drawList() {
drawList() {
var list = _listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
Expand All @@ -52,28 +62,26 @@
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) {
fetchList(afterFetch) {
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.

nit: You have successfully "Promisified" it, so you don't need to keep this callback-style argument.

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

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

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

document.addEventListener('DOMContentLoaded',function(event){
var contentManager = new ContentManager();
var listManager = new ListManager();
contentManager.start();
listManager.start();
});
6 changes: 5 additions & 1 deletion homework/test/test-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ describe('Test > ', function() {
beforeEach(function() {
});

it('will test some pure functions', function() {
it('my pure testfunctions', function() {
// Write any pure function assertion here.
var a = 1;
var b = 2;
var sum = a+b;
assert.equal(3,sum);
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.

WRONG: you need to define a pure function (method) in one manager, and then call it here.

});
});