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

Show contact type icon in notifications list #196

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

titusjaka
Copy link
Contributor

There was only telegram and email icons. I fix it so we show any supported contact icon properly:

image

Copy link

@sashasushko sashasushko left a comment

Choose a reason for hiding this comment

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

Давай ещё в TagList.js подрефачим.

А ещё у нас теперь в домене есть ContactTypes, хорошо бы на них заменить магические строки. Например, в ContactValidator.js или в том же ContactTypeIcon.js. Одним словом — везде.

@titusjaka
Copy link
Contributor Author

в TagList добавил. А потом пошёл рефакторить и понял, что не понимаю, как грамотно это сделать.

@sashasushko
Copy link

sashasushko commented Nov 1, 2018

понял, что не понимаю, как грамотно это сделать

Импортнуть енум с контактами из домена, заменить хардкорные строки на енум

@titusjaka
Copy link
Contributor Author

Не всё так однозначно. В этом енуме нет контуровских сендеров, которые у нас в отдельном репозитории хранятся. Полагаю, поэтому там используются магические переменные.
Нужно либо затаскивать контуровские сендеры в енум, либо оставлять магические переменные.

Что тебе кажется более разумным?

@sashasushko
Copy link

См. коммит

@titusjaka
Copy link
Contributor Author

Ну да, так тоже можно.
Я просто думал, что вот это if (type.includes("sms")) тоже стоит выпилить. Не знаю, как правильно сделать с учётом специфичных для контура велосипедов.

@titusjaka titusjaka merged commit 3e0ec3f into develop Nov 2, 2018
@titusjaka titusjaka deleted the feature/notification-list-icons branch November 2, 2018 07:58
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