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 #4

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

Homework5 #4

wants to merge 3 commits into from

Conversation

Pobedos
Copy link
Collaborator

@Pobedos Pobedos commented Nov 20, 2021

No description provided.

}

createSkill(id) {
const skillItem = 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.

Архитектурное начало хорошее, очень даже, давай еще немного сгруппируем логику. У нас есть Контроллер добавления и удаления скилов, есть скилы. Надо сделать SkillView, чтобы там была только логика отрисовки скила, перерисовки можно сделать на любой чих пых полные, то есть добавл или удалил элемент, перерисовал все что есть в HTML, но лучше конечно оптимизировать немного, мы жа знаем когда удаление а когда добавление. И можем в целом и связь DOM -> Модель дердать где-то, чтобы если удалился из массива элемент удалять и в DOM

- use typescript now
@Pobedos
Copy link
Collaborator Author

Pobedos commented Dec 9, 2021

Постарался переделать, используя typescript

import SkillController from "./scripts/SkillController";
import './scripts/Switcher';

const addSkillBtn: HTMLElement = document.getElementById('add-skill');
Copy link
Member

Choose a reason for hiding this comment

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

Тут тип не обязательно указывать, по идее getElementById вернет нужный тип автоматически

Copy link
Member

Choose a reason for hiding this comment

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

Првоерил, он возвращает просто Element, тип можно оставить

skillItemInner.appendChild(skillSubtitle);
skillSubtitle.appendChild(skillName);
skillSubtitle.appendChild(skillRatio);
skillItemInner.appendChild(progressbar);
Copy link
Member

@BusinessDuck BusinessDuck Dec 13, 2021

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 в качестве альтернативы можно попробовать рассмотреть темплейт, по крайней мере знай, что так тоже можно

import Skill from "./Skill";

class SkillController {
readonly startSkills: Skill[] = [
Copy link
Member

@BusinessDuck BusinessDuck Dec 13, 2021

Choose a reason for hiding this comment

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

можно startSkills просто положить в skills в конструкторе пройтись по нему и отрисовать все

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.

Мне нравится как получилось, и визуально и по горячим клавишам все работает, ты молодец.

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

@BusinessDuck
Copy link
Member

Мне нравится как получилось, и визуально и по горячим клавишам все работает, ты молодец.

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

Вот о чем я

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

@BusinessDuck
Copy link
Member

Но мне нравится как получилось, можно оставить и так.

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.

Все круто 40/40

Разберись с импортами когда стоит использовать export default, а когда просто export

+15 баллов раскидал на остальные дз

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