Skip to content

102502505 #34

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

102502505 #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c1bfad
homework_ver1
amm040341 Oct 23, 2015
7a4672a
homework_ver2--add test
amm040341 Oct 23, 2015
6705670
homework_ver3-修改Test
amm040341 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
51 changes: 31 additions & 20 deletions homework/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
'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) {

function resetWrapper() {
_wrapper.innerHTML = '';
}
var ContentManager = function() {
var _wrapper = null;
};

ContentManager.prototype = {

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

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

handleEvent(event) {
switch(event.type) {
case 'note-open':
var note = event.detail;
this.resetWrapper();
this.drawNote(note);
break;
}
},

start() {
this._wrapper = document.querySelector('#note-content-wrapper');
window.addEventListener('note-open', 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.

Good. You noticed it's necessary to bind the this as the listener.

}
};

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 @@ -3,6 +3,7 @@
<head>
<meta charset="UTF-8">
<title> Homework - Note List </title>
<script defer 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
105 changes: 59 additions & 46 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,37 @@
'use strict';

(function() {
(function(exports) {

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

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

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) {
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 +45,48 @@
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) {
var listData = this.response;
// The flow ends here.
afterFetch(listData);
} else if (this.status !== 200 ){
// Ignore error in this case.
}
};
xhr.send();
}
fetchList(url) {
return new Promise(function(reslove, 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', url, 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.
reslove(listData);
} else if (this.status !== 200 ){
reject(new Error('FETCHING FAILED: ' + this.status + ' ' + this.readyState));
}
}).bind(xhr);
xhr.send();
});
},

document.addEventListener('DOMContentLoaded', function(event) {
start();
});
handleEvent(event) {
switch(event.type) {
case 'click':
this.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 the on to a handler of corresponding event. So it in fact should be renamed as onNoteClick or onClick. It's a hidden issue about expressiveness.

break;
}
},

start() {
this._wrapper = document.querySelector('#note-list-wrapper');
this.fetchList('./demo-list-notes.json').then((function(response) {
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.

I recommend to use Arrow function instead of call bind:

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

this.updateList(response);
this.drawList();
this.preloadFirstNote();
}).bind(this)).catch(function(error){
console.error(error);
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 serious issue that you catch it but don't re-throw it. To log it here is to prevent the following chains make it silent, while to re-throw it is to make sure the following chains outside the method will get interrupted.

});
window.addEventListener('click', this);
}
};

})();
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();
});
15 changes: 10 additions & 5 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() {
beforeEach(function() {
});
describe('Test Math.sin', function() {
var subject;

beforeEach(function() {
subject = new TestManager;
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: why did you think an inexistent instance could be test like this? You should discover the issue if you ever tried to run it. See my file in this repo about how to run the test.

});

it('will test some pure functions', function() {
// Write any pure function assertion here.
it('will test sin function', function() {
subject.set(Math.PI);
assertEqual(subject.sin(),0);
done();
});
});