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

Homework5_Kiselyov_Alexander #6

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Homework5_Kiselyov_Alexander #6

wants to merge 6 commits into from

Conversation

AlexanderKiselyov
Copy link
Collaborator

No description provided.

@AlexanderKiselyov
Copy link
Collaborator Author

Проверять необходимо только последний коммит.

gulpfile.js Outdated
const bundle = await rollup.rollup(rollupConfig);

bundle.write({
format: 'esm',
Copy link
Member

Choose a reason for hiding this comment

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

В чате написал только что что тут можно поменять на iife и свободно использовать import в js файлах. Сделай пожалуйста, а то очень большие JS файлы

Copy link
Member

Choose a reason for hiding this comment

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

Круто что ты заморочился роутингом, роутер стоит сделать отдельным модулем точно , чтобы не путался
под ногами в общих файлах. Кстати есть у меня какой-то роутер, может будет чем-то полезно https://github.com/BusinessDuck/html5-history-router

Copy link
Member

Choose a reason for hiding this comment

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

примерная структура может выглядеть вот так

/src/
     index.js // Файл который запускает приложение
     /skills/
         - skill.js
         - utils.js
         - ...
     /themes/
     ....
         

src/js/skills.js Outdated

skills.onclick = function () {
pathParts = window.location.pathname.split("/");
if (pathParts[pathParts.length - 1] == "skills.html") {
Copy link
Member

Choose a reason for hiding this comment

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

про роутинг это вот это

src/js/skills.js Outdated
}

function addNewSkill(skillNameNew, skillPercentNew) {
var skillsWrapper = document.getElementById("card__skills-wrapper");
Copy link
Member

Choose a reason for hiding this comment

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

Просто для справки, можно еще было вот таким образом сделать https://developer.mozilla.org/ru/docs/Web/HTML/Element/template

src/js/skills.js Outdated
@@ -0,0 +1,68 @@
document.addEventListener("DOMContentLoaded", function(event) {
var skillsButton = document.getElementById("skills");
Copy link
Member

Choose a reason for hiding this comment

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

можно и нужно использовать let/const вместо var

src/js/skills.js Outdated
var newSkillName = document.getElementById("newSkillName");
var newSkillValue = document.getElementById("newSkillValue");
addNewSkillEl.onclick = function () {
if (newSkillName.value && newSkillValue.value && newSkillValue.value >= 0 && newSkillValue.value <= 100) {
Copy link
Member

Choose a reason for hiding this comment

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

нужно обрабатывать событие формы, а не кник по кнопке https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/formdata_event

Copy link
Member

@BusinessDuck BusinessDuck left a comment

Choose a reason for hiding this comment

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

Посмотри пример в ООП, но можно сделать и по своему.

Исправить замечания, определиться с подходом функционально или ООП, обязательно разделить файлы JS на составные и использовать import. Не держать всю логику в DOMContentLoaded обработчике.

Сейчас в основном применяется в разработке реактивный подход. Это когда состояние модели описано отдельно, а UI изменяется в зависимости от изменения модели. То есть у тебя есть массив данных в нашем случае это скилы, в него добавляются или удаляются скилы, и в зависимости от вида манипуляции с моделью DOM реагирует на это и перестраивается, вот ниже пример как этого можно добиться. Однако будет ряд трудностей, во первых не понятно как грамотно перерисовывать содержимое, потому что может удалиться элемент в середине, и как-то нужно вычислить, какой это элемент и удалить его точечно, также элемент может добавиться в конец или в начало (не в нашем случае а в общем). Все эти перерисовки сложно управляемы. Эту задачу решают фреймоврки как раз.

В общем и целом, реактивных подход позволяет хранить данные модели отдельно, тем самым получая к ним доступ из любых мест приложения и делая его более гибким. Для отслеживания изменений в модели чаще всего используется паттерн Observer, ссылка и описание ниже.

Вот еще несколько примеров реализации от меня

export class Skill {
    constructor(readonly name: string, readonly value: number) {}
}

export class SkillList {
    private list: Array<Skill>;

    constructor() {
        this.list = [];
    }

    add(skill: Skill) {
        this.list.push(skill);
    }

    remove(skill: Skill) {
        this.list.splice(this.list.indexOf(skill), 1);
    }
}

export class SkillView {
    constructor(private skill: Skill) {}

    render() {
        return this.HTML(`<div class="skill">${this.skill.name}<span>${this.skill.value}</span></div>`);
    }

    private HTML(str: string) {
        // @ts-ignore
        return repalceHtmlEntities(str);
    }
}

export class SkillListView {
    private list: Array<SkillView>;

    render() {
        return this.list.map(skill => skill.render()).join('');
    }
}

export class SkillListController {
    private skillList: SkillList;
    private skillListView: SkillListView;

    constructor(private target: Element) {...}

    addSkill(...) {
        // ...
        const skill = new Skill('123', 33);
        this.skillList.add(skill);
        this.target.innerHTML = this.skillListView.render();
    }
}

// index.ts

const target = document.querySelector('.target');
const skillController = new SkillListController(target);

// В идеале вообще использовать Observer Pattern

// index2.ts
const target = document.querySelector('.target');
const model = new SkillList();
const skillController = new SkillListController(target, model);

function addSkill() {
    model.push({ name: 'asdf', value: 33 });
}

function removeSkill(idx) {
    model.removeSkill(idx);
}

// И где-то внутри контроллера скилов
// Мы подписались на изменения модели, то есть при добавлении и удалении элемента вызовется коллбек. Паттерн 
 Observer можно посмотреть тут
https://monsterlessons.com/project/lessons/observer-pattern-v-javascript
this.model.subscribe((model => {});

skillWrapper.style.margin = "10px";
let skillName = document.createElement("div");
skillName.innerText = skillNameNew;
let skillProgress = document.createElement("div");
Copy link
Member

Choose a reason for hiding this comment

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

очень много строков, не самый эффективный метод, но рабочий конечно

Copy link
Member

@BusinessDuck BusinessDuck left a comment

Choose a reason for hiding this comment

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

Что понравилось

  1. Мало кода в целом
  2. Функциональный стиль во всей красе
  3. Дизайн и переходы по страницам
  4. Попытка Роутинга пусть и не совсем правильная

Что не понравилось

  1. Не смог запустить проект
  2. type=module, но сборка iife, она не требует этот тип
  3. type: "module" в package.json не нужен он не дает нормально жить сборке
  4. Не работает сборка gulp, пришлось чинить, не правильно был задан путь и лишние типы модулей не давали ей работать, также версия пакета gulp-image, устанавливалась новая изза ^ в начале версии, и не запускалась на текущей версии gulp. Еще не импортируются pages/skills.html в сборку, не указано src/**/*.html в настройках gulp для сборки файлов html
  5. Когда все завелось в итоге скилл 2жды добавляется на сабмит
  6. Дизайн не оч, красная кнопка перебор)

Итого 25/40

@aqus aqus removed their request for review January 5, 2022 16:26
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