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

homework for lesson 4 #61

Open
wants to merge 14 commits into
base: lecture-4
Choose a base branch
from
Open

Conversation

sulakin
Copy link

@sulakin sulakin commented Dec 27, 2022

No description provided.

Copy link
Owner

@Letto228 Letto228 left a comment

Choose a reason for hiding this comment

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

Привет
Супер! Очень продуманный popup компонент получился.
Есть 2 нюанса которые смущают:

  1. Я бы рассмотрел кейс, когда в popupTemplate приходит undefined, в теории такой кейс возможен, например, когда будет сбрасываться состояние переменной в которой хранится template. Если же в твоей архитектуре такого кейса нет, то тогда можно не учитывать кейс с undefined
  2. В tempalte может быть прописан компонент, соответственно, при создании view на основе template - создастся и компонент используемый в нем. Вот в чем соль, при закрытии попапа, ожидаешь ли ты, что компонент будет уничтожен, что у него отработает хук OnDestroy, который выполнит отписки от подписок в компоненте(если они, конечно же там есть)?
    Мне как раз таки кажется, что при закрытии попапа стоит уничтожать вставленный view, так будет более корректно, т.к. нет смысла держать рабочий компонент, раз попап с ним закрыли.
    Возможно, у тебя попап используется для кейсов в которых обычное скрытие view, без его уничтожения, это подходящее решение

Буду ждать твоего ответа)

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