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.

Sign up for GitHub

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

Jump to bottom

feat(push notification carousel): Implement image loading for push notifications #17

Merged
merged 23 commits into from
Jun 13, 2024
Merged

feat(push notification carousel): Implement image loading for push notifications #17

merged 23 commits into from
Jun 13, 2024

Conversation

DanielGreenEngineer
Copy link
Contributor

@DanielGreenEngineer DanielGreenEngineer commented Jun 4, 2024 •
edited
Loading

Добавить возможность отображения карусели в мобильном пуше при помощи - Implement image loading for push notifications

@DanielGreenEngineer DanielGreenEngineer self-assigned this Jun 4, 2024
@TorinAsakura
Copy link
Member

@DanielGreenEngineer обрати внимание на свой ПР

  • первое что бросается в глаза это неправильное использование области (scope) который у тебя говорит о месте работы через фразы, а не субъект, ну вот что такое push noitfication carousel? У нас где-то есть такой модуль/файл/метод/событие? И пробелы вообще запрещены в таких местах, разделители должны обрабатываться через спец. символы
  • что такое WIP? Ты создаёшь ПР однажды, без возможности менять его имя по ходу работы - тогда зачем ты пишешь WIP? Вместо WIP - используй специальный статус ПРа - draft - находится прямо под блоком ревьюверов в сайдбаре - при условии, что ПР ещё не готов к ревью

@TorinAsakura
Copy link
Member

Также, чтобы обратить моё внимание на изменения, которые ты проделал - нужно перезапрашивать ревью - иконка с цикличными стрелками рядом с моим ником в разделе ревьюверов

@xeewii
Copy link
Contributor

xeewii commented Jun 7, 2024

@DanielGreenEngineer У нас сейчас происходит миграция на котлин, поэтому есть предложение этот ПР мержить после #18 соответственно с изменениями уже на котлине

@DanielGreenEngineer
Copy link
Contributor Author

@xeewii Привет. Логичное предложение, поддерживаю тебя

@DanielGreenEngineer DanielGreenEngineer marked this pull request as draft June 7, 2024 12:24
@DanielGreenEngineer
Copy link
Contributor Author

DanielGreenEngineer commented Jun 7, 2024 •
edited
Loading

@TorinAsakura

Первое что бросается в глаза это неправильное использование области (scope) который у тебя говорит о месте работы через фразы, а не субъект, ну вот что такое push noitfication carousel? У нас где-то есть такой модуль/файл/метод/событие? И пробелы вообще запрещены в таких местах, разделители должны обрабатываться через спец. символы.

Посмотрел пример в одном из риквестов, весомое замечание, исправлюсь.

Что такое WIP? Ты создаёшь ПР однажды, без возможности менять его имя по ходу работы - тогда зачем ты пишешь WIP? Вместо WIP - используй специальный статус ПРа - draft - находится прямо под блоком ревьюверов в сайдбаре - при условии, что ПР ещё не готов к ревью.

Всегда ошибочно считал что так можно помечать риквест в работе, в дальнейшем оставлю себе в использование только draft пометку

Также, чтобы обратить моё внимание на изменения, которые ты проделал - нужно перезапрашивать ревью - иконка с цикличными стрелками рядом с моим ником в разделе ревьюверов.

Не знал об этом, спасибо за совет

@xeewii
Copy link
Contributor

xeewii commented Jun 7, 2024

@DanielGreenEngineer Пр с миграцией в мастере, можно менять)

@DanielGreenEngineer
Copy link
Contributor Author

@xeewii Спасибо большое, затащу себе!)

@DanielGreenEngineer
Merge branch 'master' into feat/push-notification-carousel
aa7aaa7 
# Conflicts:
#	personalizatio-sdk/src/main/java/com/personalizatio/MessagingService.java
#	personalizatio-sdk/src/main/java/com/personalizatio/SDK.java
#	personalizatio-sdk/src/rees46/java/com/rees46/sdk/REES46.java
#	sample/src/main/java/com/personalizatio/sample/AbstractMainActivity.java
#	sample/src/main/java/com/personalizatio/sample/AbstractSampleApplication.java
@DanielGreenEngineer
Rename .java to .kt
4077ae5
@DanielGreenEngineer
feat(notifications): The logic for processing push notifications was …
2c608a3 
…converted from Java to Kotlin in NotificationIntentService and NotificationHelper
@DanielGreenEngineer
fix: remove SDK.java, change constants in NotificationHelper, add str…
4414e41 
…ings for notification buttons
@DanielGreenEngineer
refactor: optimized imports
25952c9
@DanielGreenEngineer
refactor: created TAG inside object NotificationHelper
c8d574b
@DanielGreenEngineer
Copy link
Contributor Author

@TorinAsakura Привет. У меня пара вопросов:

  • Заглянул в пайплайны, вижу много фейлов по тестам, стоит ли мне их все починить?
  • Нужен твой совет по поводу IntentService, он уже Deprecated, стоит ли его заменить на Worker или же нет?

@DanielGreenEngineer DanielGreenEngineer marked this pull request as ready for review June 10, 2024 13:09
@DanielGreenEngineer DanielGreenEngineer changed the title feat(push notification carousel): Implement image loading for push notifications - WIP feat(push notification carousel): Implement image loading for push notifications Jun 10, 2024
@TorinAsakura
Copy link
Member

@TorinAsakura Привет. У меня пара вопросов:

  • Заглянул в пайплайны, вижу много фейлов по тестам, стоит ли мне их все починить?
  • дождись мержа соседнего пра, повтори и если чеки всё ещё падают - давай чинить в отдельной таске, чтобы фичу не тормозить
  • Нужен твой совет по поводу IntentService, он уже Deprecated, стоит ли его заменить на Worker или же нет?
  • стоит, но, в отдельной задаче

@TorinAsakura
Copy link
Member

@DanielGreenEngineer чекай

@DanielGreenEngineer
Copy link
Contributor Author

чекай

Принято

@DanielGreenEngineer
Merge branch 'master' into feat/push-notification-carousel
bddf50a 
# Conflicts:
#	personalizatio-sdk/src/main/kotlin/com/personalizatio/notification/NotificationHelper.kt
#	personalizatio-sdk/src/main/kotlin/com/personalizatio/notification/NotificationIntentService.kt
#	personalizatio-sdk/src/rees46/kotlin/com/rees46/sdk/REES46.kt
#	sample/src/main/kotlin/com/personalizatio/sample/AbstractMainActivity.kt
#	sample/src/main/kotlin/com/personalizatio/sample/AbstractSampleApplication.kt
TorinAsakura
TorinAsakura requested changes Jun 11, 2024
View reviewed changes
personalizatio-sdk/src/main/kotlin/com/personalizatio/SDK.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/SDK.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/SDK.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/SDK.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/SDK.kt Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/notification/NotificationHelper.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/notification/NotificationHelper.kt Outdated Show resolved Hide resolved
personalizatio-sdk/src/main/kotlin/com/personalizatio/notification/NotificationHelper.kt Show resolved Hide resolved
@TorinAsakura
Copy link
Member

@DanielGreenEngineer @xeewii чеки падают, нужно чинить

@DanielGreenEngineer
Copy link
Contributor Author

@DanielGreenEngineer @xeewii чеки падают, нужно чинить

Принято, починю

TorinAsakura
TorinAsakura previously approved these changes Jun 13, 2024
View reviewed changes
@TorinAsakura TorinAsakura merged commit 38c3b7a into master Jun 13, 2024
1 check passed
@TorinAsakura TorinAsakura deleted the feat/push-notification-carousel branch June 13, 2024 13:38
@gochicus gochicus mentioned this pull request Jun 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TorinAsakura TorinAsakura TorinAsakura approved these changes

Assignees

@DanielGreenEngineer DanielGreenEngineer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielGreenEngineer @TorinAsakura @xeewii