Skip to content
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.

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

task9-10 #11

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

task9-10 #11

wants to merge 23 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 12, 2018

No description provided.

Copy link

@ala-n ala-n left a comment

Choose a reason for hiding this comment

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

Я тут много чего пишу, а на самом деле это в основном уже придирки )
То что получается мне нравится.
Единственный момент осалось баги пофиксить (пока в живую не все работает ;) ).
И можно мелочи доделывать.
Сейчас на 100% есть очень неплохой скелет приложения.

package.json Outdated
"dependencies": {
"connect": "^3.6.6",
"express": "^4.16.3",
"http": "0.0.0",
Copy link

Choose a reason for hiding this comment

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

Це что? Что-то страшное похоже... (там один package.json c версией 0.0.0)

http - встроенный модуль node.js он не является сторонней зависимостью, здесь его прописывать не надо.

package.json Outdated
},
"homepage": "https://github.com/adamskaya/up#readme",
"dependencies": {
"connect": "^3.6.6",
Copy link

Choose a reason for hiding this comment

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

connect - "Connect is an extensible HTTP server framework for node using "plugins" known as middleware."
Т.е. это ресурс которы делат часть того что есть в express - это 1, и мы его не используем 2.

package.json Outdated
"connect": "^3.6.6",
"express": "^4.16.3",
"http": "0.0.0",
"multer": "^1.3.0"
Copy link

Choose a reason for hiding this comment

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

А вот это уже нужная зависимость.

Итого тут остается только express + multer

package.json Outdated
"name": "package",
"version": "1.0.0",
"description": "",
"main": "index.js",
Copy link

Choose a reason for hiding this comment

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

server.js

server.js Outdated


app.use(express.static('public'));
app.use(bodyParser.urlencoded());
Copy link

Choose a reason for hiding this comment

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

app.use(bodyParser.urlencoded({extended: true}));

Чтобы express не ругался на тот факт что в следующей версии bodyParser дефолтом будет extended: false и это может повлиять на правильную работу приложения.

public/DOM.js Outdated
}
}

class newPostView {
Copy link

Choose a reason for hiding this comment

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

С большой буквы

public/DOM.js Outdated
class newPostView {
static get msgTemplate() {
if (!newPostView._template) {
newPostView._template = document.getElementById('add-post-template');
Copy link

Choose a reason for hiding this comment

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

А нужно ли нам сокрывать в темплейт форму добавления / регистрации(выше) если они в единственном числе и скрыты вместе с секцией другим образом.

Copy link

Choose a reason for hiding this comment

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

Я помню что я сказал что можно в темплейт поместить, но я не знал контекста. Например если бы мы добаляли форму авторизации прямо в шапку, в этом был бы смысл, а тут у нас в единственном числе в определенном месте, скрытым другим образом.
Но это я в целом, это точно не ошибка, это просто заметка о том как могло быть.

public/DOM.js Outdated

/**/
class RegistrationFormView {
static get msgTemplate() {
Copy link

Choose a reason for hiding this comment

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

везде msgTemplate, хотя не msg (message) это вовсе.

Copy link

Choose a reason for hiding this comment

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

можно просто template, чтоб вопросов не возникало к названию ;)


// =============================================
async function load() {
await model.loadPhotoPosts();
Copy link

Choose a reason for hiding this comment

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

Вот а про это поговорим на паре, здесь один интересный момент получается...
Запросы уйдут последовательно,а могли уйти параллельно

server.js Outdated
app.use(bodyParser.urlencoded());
app.use(bodyParser.json());

function getPhotoPost(id) {
Copy link

Choose a reason for hiding this comment

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

Бекэнд код кстати можем смело разделять на отдельные части, например вынести бизнесс логику в отдельный node модуль (https://metanit.com/web/nodejs/2.1.php), а в основном файлике оставить только сам сервер.

<form id="newEntry" name="entry">
<div class="download-photo">
<p>download photo</p>
<button class="button-add-photo" type="submit">
Copy link

Choose a reason for hiding this comment

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

newElemListView.clearPage();
newElemListView.addFormForNewPost();
setFormPostData(post);
document.getElementById('button-submit-post').addEventListener('click', async () => {
Copy link

Choose a reason for hiding this comment

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

Лучше когда речь идет о работе с формами пологаться на событие формы submit.
Плюс здесь мы вешаем событие но не делаем его диночным или открепляем его.
Во второй раз у нас лисененров будет 2, дальше 3 и так далее, и каждый из них отправит запрос.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants