Lecture-4 by AK20001701 · Pull Request #76 · Letto228/Angular-learnjs-121222

AK20001701 · 2023-01-07T01:08:51Z

No description provided.

AK20001701 · 2023-01-07T01:12:30Z

@@ -0,0 +1,13 @@
+<!-- <div class="popup"> -->


Возникла проблема, и какого-то адекватного решения не нашел. Я добавил стили для отобрадения popup в центре. Первое что я сделал, это обернул ng-container в <div class="popup">, но в таком случае при отсутствии содержимого шаблона выводится пустой элемент со стилем (получается пустая светлая точка в центре экрана). Потом решил попробовать сделать контейнер в шаблоне в котором уже будет <div class="popup">, но с данным вариантом тоже не получилось, и как-то он странно выглядит в любом случае. Как надо было поступить в таком случае?

Ты шел в правильном направлении, стоит продолжить идею с div оберткой - #76 (comment)

Рабочего результата я добился.
Но не уверен что все красиво получилось.
Первый вопрос, я получается должен был добавить в .less .hide { display: none; } ?
И правильно ли я понимаю, если я вызову из AppComponent this.popupHost.popupTemplate = this.templateTwo;, то ngOnChanges не отработает в PopupHostComponent?

И насколько корректно мое решение с _popupTemplate?

Работа в компоненте PopupHostComponent выстроена корректно за исключением пары нюансов:

Я бы не стал делать сеттер, раз тебе нужно хранить свойство полученное в инпуте, рекомендовал бы в таком случаи popupTemplate сделать обычным свойством, а изменения отслеживать через OnChanges хук.
Тот подход, что ты использовал с сеттером и _popupTemplate свойством, тоже подходит и сторонников такого подхода не мало. Но в таком случаи смущает момент: свойство с _ обычно используется для нейминга приватных свойств, а в твоем случае данное свойство публично, если поправить нейминг, то будет все супер.

По поводу closePopup метода. Сейчас компонент управляется за счет инпут свойства(передали значение TemplateRef - отображаем, передали undefined - скрываем), то есть то, что отображать - диктует родительский компонент.
При закрытии попапа через метод closePopup нарушается архитектура описанная выше, т.к. дочерний компонент начинает сам управлять своим отображением и при этом нарушает консистентность своего состояния: Angular по прежнему считает, что в инпуте хранится значение отображаемого шаблона, а тем временем внутреннее свойство имеет значение undefined(Подобный кейс с рассинхронизацией мы рассматривали на 3 лекции, если не ошибаюсь, когда создавали sidenav компонент). По этому, если ты хочешь реализовать закрытие из PopupHostComponent по той архитектуре, что сейчас есть, то нужно создать Output, который будет сообщать родителю о необходимости закрыть popup, соответсвенно и вся логика будет реализовываться у родителя.

Ответы на твои вопросы:

получается должен был добавить в .less .hide { display: none; } ?

Да, все верно

если я вызову из AppComponent this.popupHost.popupTemplate = this.templateTwo;, то ngOnChanges не отработает в PopupHostComponent?

Да, тут тоже все верно, ngOnChanges отрабатывает только при изменении свойства через механизм CD, а у тебя оно меняется через JS.
В целом, не очень хорошая практика так менять Input свойства, потому что это идет в обход механизма CD и флоу общения компонентов в парадигме Angular. Попробуй изменить этот момент - убрать любое обращение к PopupHostComponent из класса AppComponent и выстроить всю работсу через Input и Output.
P.S. К popupTemplate инпуту можно привязать свойство из AppComponent, назовем его, например, popupTemplate. Значение свойства popupTemplate в AppComponent можно будет изменять как угодно и значение будет передано в инпут по флоу Angular: <app-popup-host [popupTemplate]="popupTemplate"></app-popup-host>; closePopup метод в AppComponent в таком случае будет выглядить так closePopup() { this.popupTemplate = undefined; }

Letto228 · 2023-01-10T06:10:48Z

Привет!
Давай подброшу идею: <div class="popup"> можно скрыть при отсутсвии template(template === undefined).
Реализовать скрытие можно при помощи классов и стилей - [class.hide] или при помощи структурной директивы ngIf

Letto228

Добавил еще пару комментариев, которые заметил, для проведения полноценного ревью - буду ждать обновления МР

Letto228 · 2023-01-10T06:15:13Z

+	private popupContainer!: ViewContainerRef;
+
+	@Input() set popupTemplate(popupTemplate: TemplateRef<unknown>) {
+		this.popupContainer?.clear();


Для this.popupContainer optional chaining не нужен, т.к. контейнер статичен на странице

У меня optional chaining все еще отображается 🤔

Letto228 · 2023-01-10T06:16:29Z

+	@ViewChild('popupContainer', { read: ViewContainerRef, static: true })
+	private popupContainer!: ViewContainerRef;
+
+	@Input() set popupTemplate(popupTemplate: TemplateRef<unknown>) {


В popupTemplate может еще придти и undefined, давай обработаем этот кейс

Добавил проверку, в случае undefined не создаю EmbeddedView. Или как лучше обработать такой случай?

Огонь, все по феншую, только давай еще в типизации отобразим, что может придти undefined

Letto228

Отписался по твоим правкам в комментариях, посмотри пожалуйста. Постарался все максимально подробно изложить, но если все таки что то будет не ясно, то обязательно напиши в Дискорде - договоримся о созвоне и на нем все разложим по полочкам

Letto228 · 2023-01-15T16:57:21Z

@@ -0,0 +1,13 @@
+<!-- <div class="popup"> -->


Работа в компоненте PopupHostComponent выстроена корректно за исключением пары нюансов:

Я бы не стал делать сеттер, раз тебе нужно хранить свойство полученное в инпуте, рекомендовал бы в таком случаи popupTemplate сделать обычным свойством, а изменения отслеживать через OnChanges хук.
Тот подход, что ты использовал с сеттером и _popupTemplate свойством, тоже подходит и сторонников такого подхода не мало. Но в таком случаи смущает момент: свойство с _ обычно используется для нейминга приватных свойств, а в твоем случае данное свойство публично, если поправить нейминг, то будет все супер.

По поводу closePopup метода. Сейчас компонент управляется за счет инпут свойства(передали значение TemplateRef - отображаем, передали undefined - скрываем), то есть то, что отображать - диктует родительский компонент.
При закрытии попапа через метод closePopup нарушается архитектура описанная выше, т.к. дочерний компонент начинает сам управлять своим отображением и при этом нарушает консистентность своего состояния: Angular по прежнему считает, что в инпуте хранится значение отображаемого шаблона, а тем временем внутреннее свойство имеет значение undefined(Подобный кейс с рассинхронизацией мы рассматривали на 3 лекции, если не ошибаюсь, когда создавали sidenav компонент). По этому, если ты хочешь реализовать закрытие из PopupHostComponent по той архитектуре, что сейчас есть, то нужно создать Output, который будет сообщать родителю о необходимости закрыть popup, соответсвенно и вся логика будет реализовываться у родителя.

Letto228 · 2023-01-15T17:12:53Z

@@ -0,0 +1,13 @@
+<!-- <div class="popup"> -->


Ответы на твои вопросы:

получается должен был добавить в .less .hide { display: none; } ?

Да, все верно

если я вызову из AppComponent this.popupHost.popupTemplate = this.templateTwo;, то ngOnChanges не отработает в PopupHostComponent?

Да, тут тоже все верно, ngOnChanges отрабатывает только при изменении свойства через механизм CD, а у тебя оно меняется через JS.
В целом, не очень хорошая практика так менять Input свойства, потому что это идет в обход механизма CD и флоу общения компонентов в парадигме Angular. Попробуй изменить этот момент - убрать любое обращение к PopupHostComponent из класса AppComponent и выстроить всю работсу через Input и Output.
P.S. К popupTemplate инпуту можно привязать свойство из AppComponent, назовем его, например, popupTemplate. Значение свойства popupTemplate в AppComponent можно будет изменять как угодно и значение будет передано в инпут по флоу Angular: <app-popup-host [popupTemplate]="popupTemplate"></app-popup-host>; closePopup метод в AppComponent в таком случае будет выглядить так closePopup() { this.popupTemplate = undefined; }

Letto228 · 2023-01-15T17:16:02Z

+	private popupContainer!: ViewContainerRef;
+
+	@Input() set popupTemplate(popupTemplate: TemplateRef<unknown>) {
+		this.popupContainer?.clear();


У меня optional chaining все еще отображается 🤔

Letto228 · 2023-01-15T17:17:37Z

+	@ViewChild('popupContainer', { read: ViewContainerRef, static: true })
+	private popupContainer!: ViewContainerRef;
+
+	@Input() set popupTemplate(popupTemplate: TemplateRef<unknown>) {


Огонь, все по феншую, только давай еще в типизации отобразим, что может придти undefined

feat: add popup host

791cb1e

AK20001701 commented Jan 7, 2023

View reviewed changes

Letto228 requested changes Jan 10, 2023

View reviewed changes

akazanin added 5 commits January 14, 2023 18:01

fix: hide popup after closing

ee347f0

refactor: optional chaining, add undefined check

0aded30

refactor: optional chaining, add undefined check

84afdbc

refactor: delete onChanges

79c170e

refactor: delete extra clear call

38190a7

Letto228 requested changes Jan 15, 2023

View reviewed changes

Conversation

AK20001701 commented Jan 7, 2023

Uh oh!

AK20001701 Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Letto228 commented Jan 10, 2023

Uh oh!

Letto228 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Letto228 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AK20001701 Jan 7, 2023 •

edited

Loading