Skip to content

Homework 102502043 #40

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

Homework 102502043 #40

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
e82af75
Bind this in Event handlers
Oct 28, 2015
269bcdc
'Deal with object properties w/ constructor & prototype
Oct 28, 2015
3e76722
Deal with asynchronous flows with Promise
Oct 28, 2015
3ecd4cf
My Test
Oct 28, 2015
9cb3fe7
Modify Deal with object porperties constructor&protptype
Oct 30, 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
67 changes: 35 additions & 32 deletions homework/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,40 @@
'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 resetWrapper() {
_wrapper.innerHTML = '';
(function(exports) {
var ContentManager = function () {
this._wrapper = document.querySelector('#note-content-wrapper'}

}

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);
}
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.

Instead of binding an anonymous function, you should bind the this as the listener. And Arrow function is better than the binding API (it's more concise), although I didn't require you to use that. See:

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

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

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.appendChid(p);
});
this._wrapper.appendChid(h);
this._wrapper.appendChid(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"></scipt>
<link rel="stylesheet" type="text/css" href="main.css" />
</head>
<body>
Expand Down
144 changes: 74 additions & 70 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,79 +1,83 @@
'use strict';

(function() {
(function(exports) {

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

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

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 }));
};
}

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

function updateList(list) {
_listNoteContent = list;
}
ListManager.prototype = {
start(){
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 just wonder why your indentation looks so weird...

this.fetchList()
.then((function)(data){
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.

(function)(data)

this.updateList(data);
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 here: please remember to log AND re-throw Promise error. To log it is because you can know the error early and in your own format (also, to prevent it being captured improperly by the following chain), and to re-throw it is to prevent the other chains concating it keep running after the error.


}).bind(this) );

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

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);
}
onNoteOpen(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's a convention to prefix method with on for the corresponding function, so it should be renamed as 'onClick'

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

preloadFirstNote(){
if(this.listNoteContent.lenght !== 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.

If a function is only with one if...else and there is no else branch after all, it's better to move the condition out of the callee.

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

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();
}

document.addEventListener('DOMContentLoaded', function(event) {
start();
drawList(){
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 really need to correct your indentation...

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;
buff.appendChild(li);
});
ul.appendChild(buff);
this._wrapper.appendChild(ul);
},

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" it.

var xhr = new XMLHttpRequest();
xhr.open('GET','http://127.0.1:8000/demo-list-notes.json',true);
xhr.responseType = 'json';
xhr.onreadystatechange=function(e){
if(xhr.readyState==&& xhr.status==200){
resolve(xhr.response);
}else if (xhr.status !=200){
reject(xhr);
}
};
xhr.send();
});
}
}


})();
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();
});
11 changes: 8 additions & 3 deletions homework/test/test-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
describe('Test > ', function() {
var assert = require('assert');

describe('Test Math',function(){
beforeEach(function() {
});

it('will test some pure functions', function() {
// Write any pure function assertion here.
it('Calculate the Maximum num', function() {
var small = 1;
var big = 10;
var maxNumber = Math.max(big,small);
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's better to write a method of manager and then call it here.

assert.equal(maxNumber,big);
});
});