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

HW5 #8

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

HW5 #8

wants to merge 5 commits into from

Conversation

LutsenkoDm
Copy link
Collaborator

No description provided.

@@ -1,9 +1,98 @@
document.addEventListener("DOMContentLoaded", function(event) {
document.querySelector("#portfolio-card-theme-switcher").addEventListener('click', changeTheme);
Copy link
Member

Choose a reason for hiding this comment

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

У тебя отличная реализация с точки зрения JS без фреймворков, Именно так писали раньше до реакта и классов. С появлением классов все стали делать реактивные компоненты. Почему решил именно в функциональном стиле написать?

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
Collaborator Author

Choose a reason for hiding this comment

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

Вообще изначально я хотел сделать, что-то типа класса генератора, но в итоге все выливалось в конструктор без параметров и один осмысленный метод, - generate. И я решил, сделать на функциях. Плюсы - удобно вносить мелкие правки, при этом можно быть уверенным, что ничего не сломается и не будет side эффектов, я имею в виду, например, изменить что-то для процента или name, просто дописываем / меняем configureName или configurePercent и не беспокоимся про все остальное, конечно, подобное справедливо на 100% только для чистых функций. Также из плюсов, простота кода и разработки, если делать все аккуратно (сохранять одинаковую структуру для всех функций и т.п.). Возможно еще быстрее будет работать, все-таки проще уже сложно сделать. По поводу минусов, я старался разделить, хоть и условно логику и отображение, то есть в начале всегда идут классы, потом методы типа configure, потом уже все eventListener`s. Я думал, еще сделать еще какой-то mapping, к примеру для сохранения формы, чтобы инпут был ключем, а то что его подменяет значением, а потом в цикле просто пройти и в одной строке все заменять, вроде это бы "сгладило минусы", но решил оставить как есть.

Copy link
Member

Choose a reason for hiding this comment

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

Мне больше по душе реактивность, но я готов принять аргументирований выбор. Сделай разделение по файлам, в js, за тайпскрипт накину ещё 15 баллов, если импорты в браузере не заработают, поменяй в rollup, "esm" на "iife" и все заработает. В функциональном стиле важна разбивка на файлы и структурность и логика кода.

ps. Вариант с реактивностью архитектурно бы выглядел так: есть класс скилл, и модель данных состоящая из массива скилов. Есть контроллер отвечающий за добавление и удаление элементов туда, он не взаимодействует с дом деревом. есть классы SkillListView и SkillView, они занимаются отрисовкой данных из модели в DOM. При изменении даннных в модели, SkillListView перерисовывает все, или только то что изменилось. Это FYI

Copy link
Collaborator Author

@LutsenkoDm LutsenkoDm Dec 10, 2021

Choose a reason for hiding this comment

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

Дело в том, что я это писал сразу после первой лекции, на которой эту домашку выдали, соответственно про реактивность тогда вроде не упоминалось. Переделал на typescript, немного изменил архитектуру так как мне кажется, что если бы я просто переписал, что уже есть, то различия между js и typescript были бы минимальны, поэтому добавил классы, но в концептуальном плане все осталось +-также, разве что повысилась возможность повторного использования кода. Еще по тайпскрипту возникло пару вопросов:

  1. Нужно ли всегда указывать тип переменной
  2. Принято ли делать поля публичными или работать через геттеры и сеттеры. Например, у меня в классе SkillForm все поля приватные, геттеры и сеттеры я не делал, так как они мне не нужны были, но в общем, если бы мне они понадобились, то как лучше сделать через publc или get/set?
  3. Есть ли способ сделать replace без querySelector по классу, то есть не так написать
    this.skillForm.querySelector('.' + element.className).replaceWith(...), а как-то таким образом:
    this.skillForm.querySelector(this.saveButton).replaceWith(...) может используя что-то другое, а не querySelector, Это я к тому, что у элемента по которому делаем select может не быть класса или будет несколько с одинаковыми классами
  4. Есть ли способ вынести то, что связано с eventListener-ми куда-то отдельно и надо ли вообще это делать или то, как сделано у меня это норм,
  5. В какой ситуации использовать export с default, а когда без него

Copy link
Member

Choose a reason for hiding this comment

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

  1. Тайпскрипт не должен мешать разработке, он должен ее упрощать, так что ты волен настраивать типизацию как тебе удобно. Но в большинстве случаев лучше обьявлять тип всех данных. Хотябы поверхностно.
  2. Проще работать с публичными полями. Но все зависит от задач и реализации. В JS есть get/set методы, которые позволяют перехватывать обращения или присваивания публичных атрибутов класса, так что всегда можно их добавить, не повредив код. Поэтому мое мнение что изначально делать сеттер геттер кастомным методом смысла нет
  3. Можно, ты можешь хранить ссылку на элемент в поле класса или переменной и получить быстрый доступ к элементу без querySelector, https://developer.mozilla.org/ru/docs/Web/API/Node/replaceChild
  4. В целом можно и так
  5. Лично я предпочитаю никогда не использовать export default, поскольку через обычный поиск потом не найти этот код, тк программист может произвольное имя задать этому экспорту. Но обычно если есть много экспортов из файла, то дефолтным делают основной, если такой есть.

{qualifierName: 'placeholder', value: 'text'},
{qualifierName: 'type', value: 'Name'},
{qualifierName: 'pattern', value: '^[A-z][a-z\\s]*$'},
{qualifierName: 'required', value: 'true'},
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.

Реализация интересная, со вкусом, отличается от той, что я советую делать, но в целом не плоха. Оставь как есть. Если ответы на вопросы помогут еще немного переделать и улучшить, у тебя есть 2 дня

@LutsenkoDm
Copy link
Collaborator Author

LutsenkoDm commented Dec 17, 2021

Реализация интересная, со вкусом, отличается от той, что я советую делать, но в целом не плоха. Оставь как есть. Если ответы на вопросы помогут еще немного переделать и улучшить, у тебя есть 2 дня

Заменил export default на export, и сделал replace как в ссылке, что вы скинули

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. Валидация
  5. Тайпскрипт + 15баллов

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

  1. Стили и верстка
  2. Скилы получились самостоятельными и не встраиваются в проект визитки визуально и цвета и все остальное отличается от общей темы визитки

Итог: 40 баллов из 40

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