Skip to content

Enhancenment (PromoBanner): Render CTA Button and Avatar optionally#3697

Merged
inomdzhon merged 5 commits intoVKCOM:masterfrom
obviouslymilk:943-promo-fix
Dec 5, 2022
Merged

Enhancenment (PromoBanner): Render CTA Button and Avatar optionally#3697
inomdzhon merged 5 commits intoVKCOM:masterfrom
obviouslymilk:943-promo-fix

Conversation

@obviouslymilk
Copy link
Copy Markdown
Contributor

@obviouslymilk obviouslymilk commented Dec 1, 2022 •
edited by inomdzhon
Loading

Добавил опциональную отрисовку кнопки и аватара (+ тесты) в соответствие этому комментарию из #943

Говорят, что новичкам полезно помогать в open-source проектах тут и там. Этим и занимаюсь. Простите, если этот спойлер нарушает правила оформления реквестов. 🐢

@obviouslymilk obviouslymilk requested a review from a team as a code owner December 1, 2022 11:50
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Dec 1, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fcddb88:

Sandbox Source
VKUI - default example Configuration

inomdzhon
inomdzhon reviewed Dec 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

Отлично!!! 👍

Спасибо за тесты 🙏 🙏 🙏

Комментирую комментарий под "спойлером", ты всё сделал так, как описано в CONTRIBUTING.md 😁

@obviouslymilk
Copy link
Copy Markdown
Contributor Author

obviouslymilk commented Dec 1, 2022

Огромное спасибо @inomdzhon за комментарии 😊! Исправил часть кода, но вот это предложение заставило очень сильно подумать.

Насколько я понимаю, функция требует передачи существующего объекта HTMLElement | SVGElement. По моей логике (ещё до первого знакомства с функцией), я должен был передать и тест показал-бы, есть ли он в иерархии PromoBanner'a. Оказалось, всё не так просто 🤔

obviouslymilk
obviouslymilk commented Dec 1, 2022
View reviewed changes
@obviouslymilk
Copy link
Copy Markdown
Contributor Author

obviouslymilk commented Dec 1, 2022 •
edited
Loading

Заменил классовые запросы на поиск компонентов по их testid. Cогласен, что при изменении кода дочерних компонентов, такой вариант может быть оптимальнее.

inomdzhon
inomdzhon reviewed Dec 2, 2022
View reviewed changes
inomdzhon
inomdzhon approved these changes Dec 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@inomdzhon inomdzhon merged commit d29d1eb into VKCOM:master Dec 5, 2022
@inomdzhon inomdzhon linked an issue Dec 5, 2022 that may be closed by this pull request
[Enhancenment][PromoBanner]: завязать показ аватраки/кнопки наличие параметры iconLink/craText из bannerData #3709
Closed
@inomdzhon inomdzhon mentioned this pull request Dec 5, 2022
Closed
@inomdzhon
Copy link
Copy Markdown
Contributor

inomdzhon commented Dec 5, 2022

Ещё раз спасибо 🙏
Зареизим в ближайщем резиле

А ты мог бы написать производный комментарий к этому issue #3709? 🙏 Сейчас я не могу прикрепить тебя к нему как исполнителя, но может если ты будешь участником треда, то смогу

@obviouslymilk obviouslymilk deleted the 943-promo-fix branch December 8, 2022 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inomdzhon inomdzhon inomdzhon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@obviouslymilk @inomdzhon