IOS-8357 IOS-8158 IOS-8358 [Onramp] Country + Currency + Settings #4155

amuraveinik · 2024-11-04T10:45:36Z

Tangem/Modules/OnrampCountrySelector/OnrampCountrySelectorView.swift

Tangem/Modules/OnrampCurrencySelect/OnrampCurrencySelectorView.swift

amuraveinik · 2024-11-04T10:46:55Z

Tangem/Modules/OnrampSettings/OnrampSettingsView.swift

+                rowView
+            }
+
+            Text("Please select the correct country to ensure accurate payment options and services.")


TODO: Lokalise onramp_settings_residence_description

Вообще это можно сделать через GroupedSection, там есть footer: который везде через DefaultFooterView делаем

Я хотел тут использовать по аналогии как в настройках приложения, но:

GroupedSection не подходит тут по отсупам.

DefaultRowView не подходит тут по шрифту

DefaultFooterView не подходит тут по шрифту

Tangem/Modules/OnrampSettings/OnrampSettingsView.swift

amuraveinik · 2024-11-04T10:49:00Z

Tangem/Modules/Send/UI/Onramp/OnrampView/OnrampView.swift

-                namespace: .init(id: namespace.id, names: namespace.names)
+                namespace: .init(id: namespace.id, names: namespace.names),
+                selectCurrency: {
+                    viewModel.router?.openOnrampCurrencySelectorView()


@Balashov152 Тут нужен комментарий, архитектурно не понимаю куда этот вызов положить.
По идее у OnrampAmountView есть своя вьюмодель, но как оттуда вызвать роутер я не понимаю

не, точно не тут

Получается надо добавить OnrampAmountRoutable с этим методом, и потом проставить router в OnrampAmountViewModel, в OnrampFlowBaseBuilder

и уже через router открыть, а сам OnrampAmountRoutable будет SendViewModel подерживать, так же как и OnrampSummaryRoutable

amuraveinik · 2024-11-04T10:49:33Z

TangemExpress/Manager/OnrampDataRepository/OnrampDataRepository.swift

 public protocol OnrampDataRepository: Actor {
    func paymentMethods() async throws -> [OnrampPaymentMethod]
    func countries() async throws -> [OnrampCountry]
    func currencies() async throws -> [OnrampFiatCurrency]
 }
+
+public extension OnrampDataRepository {
+    var popularFiats: [OnrampFiatCurrency] {


Тут как договаривались - захардкожено просто на клиенте

Я думаю лучше просто code хардкодить ["GBP", "USD"] целая модель то не нужна

Tangem/Modules/OnrampCurrencySelect/OnrampCurrencySelectorViewModel.swift

Balashov152

Вообще правильно одна задача - один MR. Плюс там есть апдейт по дизайну с error и loading стейтами. Тут много достаточно что бы хорошо посмотреть

Balashov152 · 2024-11-05T11:19:26Z

Tangem/Modules/OnrampCountrySelector/OnrampCountrySelectorView.swift

+        }
+    }
+
+    private func rowViewLabel(country: OnrampCountry) -> some View {


Лучше в отдельную view. Что то типо OnrampCountryRowView / OnrampCountryRowViewData

Balashov152 · 2024-11-05T11:20:57Z

Tangem/Modules/OnrampCountrySelector/OnrampCountrySelectorViewModel.swift

+        .catch { error in
+            // TODO: Handle error
+            return Just([])
+        }


Можно так)

Suggested change

.catch { error in

// TODO: Handle error

return Just([])

}

.replaceError(with: [])

Стало неактуально как только появились стейты ошбика

Balashov152 · 2024-11-05T11:22:05Z

Tangem/Modules/OnrampCountrySelector/OnrampCountrySelectorViewModel.swift

+final class OnrampCountrySelectorViewModel: Identifiable, ObservableObject {
+    let preferenceCountry: OnrampCountry?
+    @Published var searchText: String = ""
+    @Published private(set) var countries: [OnrampCountry] = []


И здесь будет [OnrampCountryRowViewData]

А пока загрузка идет? Надо наверное через LoadingValue<[OnrampCountryRowViewData]> делать, что бы разные View были

Balashov152 · 2024-11-05T11:25:02Z

Tangem/Modules/OnrampCurrencySelect/OnrampCurrencySelectorView.swift

+            Colors.Icon.inactive
+                .frame(width: 32, height: 4)
+                .cornerRadius(2, corners: .allCorners)
+                .padding(.vertical, 8)


Это есть уже

Suggested change

Colors.Icon.inactive

.frame(width: 32, height: 4)

.cornerRadius(2, corners: .allCorners)

.padding(.vertical, 8)

GrabberViewFactory().makeSwiftUIView()

Balashov152 · 2024-11-05T11:25:38Z

Tangem/Modules/OnrampCurrencySelect/OnrampCurrencySelectorView.swift

+        }
+    }
+
+    private func rowViewLabel(currency: OnrampFiatCurrency) -> some View {


Тоже бы в OnrampFiatCurrencyView / OnrampFiatCurrencyViewData

Balashov152 · 2024-11-05T11:32:26Z

Tangem/Modules/OnrampSettings/OnrampSettingsViewModel.swift

+        repository.preferenceCountryPublisher
+            .assign(to: &$selectedCountry)


без store(in: ) не имеет смысла же

Suggested change

repository.preferenceCountryPublisher

.assign(to: &$selectedCountry)

selectedCountry = repository.preferenceCountry

.assign(to: ) работает не так как обычные паблишеры, глянь эпл доку

https://developer.apple.com/documentation/combine/publisher/assign(to:)

хм, интересно, не знал

Balashov152 · 2024-11-05T11:35:12Z

Tangem/Modules/Send/UI/Base/SendCoordinatorView.swift

-                SendView(viewModel: rootViewModel, transitionService: .init())
-                    .navigationLinks(links)
-            }
+        NavigationView {


Send / Sell / Staking не сломались? Проверь плиз

Вроде все ок

Balashov152 · 2024-11-05T11:43:39Z

Tangem/Modules/Send/UI/Onramp/OnrampView/OnrampView.swift

-                namespace: .init(id: namespace.id, names: namespace.names)
+                namespace: .init(id: namespace.id, names: namespace.names),
+                selectCurrency: {
+                    viewModel.router?.openOnrampCurrencySelectorView()


не, точно не тут

Получается надо добавить OnrampAmountRoutable с этим методом, и потом проставить router в OnrampAmountViewModel, в OnrampFlowBaseBuilder

и уже через router открыть, а сам OnrampAmountRoutable будет SendViewModel подерживать, так же как и OnrampSummaryRoutable

Balashov152 · 2024-11-05T11:45:40Z

TangemExpress/Manager/OnrampDataRepository/OnrampDataRepository.swift

 public protocol OnrampDataRepository: Actor {
    func paymentMethods() async throws -> [OnrampPaymentMethod]
    func countries() async throws -> [OnrampCountry]
    func currencies() async throws -> [OnrampFiatCurrency]
 }
+
+public extension OnrampDataRepository {
+    var popularFiats: [OnrampFiatCurrency] {


Я думаю лучше просто code хардкодить ["GBP", "USD"] целая модель то не нужна

Balashov152 · 2024-11-05T11:46:43Z

TangemExpress/Manager/OnrampDataRepository/OnrampDataRepository.swift

+}
+
+public extension OnrampDataRepository {
+    nonisolated var countriesPublisher: AnyPublisher<[OnrampCountry], Error> {


а зачем так сложно, можно через Task во ViewModel делать
Это же не паблишер, а только один запрос - ответ

amuraveinik · 2024-11-05T15:32:05Z

В общем, я сделал выводы что лучше сразу делать все в разный PRах.
Этот уже сложно разделить, но я сделал разбивку по коммитам к каждой задаче для удобства ревьюера

Balashov152 · 2024-11-05T16:00:47Z

Tangem/App/Utilities/SearchUtil.swift

+            return items
+        }
+
+        let loweccasedSearchText = searchText.lowercased()


может еще trimmed ?

Suggested change

let loweccasedSearchText = searchText.lowercased()

let loweccasedSearchText = searchText.lowercased().trimmed()

Balashov152 · 2024-11-05T16:10:12Z

Tangem/Modules/OnrampCountrySelector/OnrampCountrySelectorViewModel.swift

+        Publishers.CombineLatest(
+            countriesSubject,
+            $searchText
+        )
+        .withWeakCaptureOf(self)
+        .map { viewModel, data in
+            let (countries, searchText) = data
+            return .loaded(
+                viewModel.mapToCountriesViewData(
+                    countries: countries,
+                    searchText: searchText
+                )
+            )
+        }
+        .receive(on: DispatchQueue.main)
+        .assign(to: &$countries)


Как то сложно получается

Можно все сделать через Task, который будет ходить за countries так как они там тоже лежат в кэше.

получается что изначально у тебя self.countries = .loading. Потом ты вызываешь метод updateView(searchText: "") и "без поиска" обновляешь View с данными из репозитория.

По подписке в методе bind() подписываешься на $searchText. и там вызываешь этот же метод updateView(searchText: searchText). Все делаешь в одном Таске, отменяя его, сохранив при этом в проперти класса. И плюс сортировку можно делать тоже в background потоке, и потом просто через await runOnMain {} обновлять список

Ну это уже вкусовщина, абсолютно никакой разницы в функционале нет.
Мне просто удобнее работать с реактивными последовательностями, потому что они тут органичнее встраиваются, например:

$searchText - это поток данных, самый что ни на есть

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

Короче после нескольких таких итераций получится что ты просто моими руками напишешь код который тебе самому больше нравится)

Либо надо зафиксировать на уровне соглашения такой подход - что в приоритете используем async/await

Balashov152 · 2024-11-05T16:11:44Z

Tangem/Modules/OnrampCountrySelector/Subviews/OnrampCountryViewData.swift

+    let isAvailable: Bool
+    let isSelected: Bool


nit: Как будто можно через enum DetailsType сделать, что бы по switch case во View проходить, но так тоже норм )

Balashov152 · 2024-11-05T16:12:58Z

Tangem/Modules/OnrampCurrencySelector/OnrampCurrencySelectorView.swift

+    var failedToLoadView: some View {
+        Spacer()
+
+        MarketsUnableToLoadDataView(


Может переименовать ее как то более общее, а то в Markets поменяют, и даже не знаю что в onramp она же юзается )

Balashov152 · 2024-11-05T16:14:13Z

Tangem/Modules/OnrampCurrencySelector/OnrampCurrencySelectorViewModel.swift

+        Publishers.CombineLatest(
+            currenciesSubject,
+            $searchText
+        )
+        .withWeakCaptureOf(self)
+        .map { viewModel, data in
+            let (currencies, searchText) = data
+            return .loaded(
+                viewModel.mapToState(
+                    currencies: currencies,
+                    searchText: searchText
+                )
+            )
+        }
+        .receive(on: DispatchQueue.main)
+        .assign(to: &$currencies)


Тут тоже самое что и в OnrampCountySelectorViewModel

#4155 (comment)

Balashov152 · 2024-11-05T16:16:47Z

Tangem/Modules/OnrampCurrencySelector/Subviews/OnrampCurrencySkeletonView.swift

+            VStack(alignment: .leading, spacing: 7) {
+                SkeletonView()
+                    .frame(width: 70, height: 12)
+                    .cornerRadiusContinuous(3)
+
+                SkeletonView()
+                    .frame(width: 52, height: 12)
+                    .cornerRadiusContinuous(3)
+            }
+        }
+        .padding(.vertical, 15)


Как то смущают 15 и 7. Хорошо бы через PixelPerfect проверить, у нас CTO любит такое замечать )

Глянь плиз в остальных местах, есть вот ответ от дизайна что должно быть 14 везде
https://www.figma.com/design/ZJoUO3kZGCcVgOUbCMQcY4?node-id=38164-85535#1013677827

Balashov152 · 2024-11-05T16:18:44Z

Tangem/Modules/OnrampSettings/OnrampSettingsViewModel.swift

+        repository.preferenceCountryPublisher
+            .assign(to: &$selectedCountry)


хм, интересно, не знал

Balashov152 · 2024-11-05T16:22:21Z

Tangem/Modules/Send/UI/Base/SendCoordinator.swift

-    @Published var onrampCountryViewModel: OnrampCountryViewModel?
+    @Published var onrampCountryDetectionViewModel: OnrampCountryDetectionViewModel?
+
+    @Published var onrampSettingsViewModel: OnrampSettingsViewModel?


А это не надо делать через отдельный coordinator? Вроде понятно что и так должно работать, но не было бы проблем на старых осях, где SUI дальше чем одна View не может

Потестил на

18.0

17.5

16.4

15.5

По этому флоу все ок

Balashov152 · 2024-11-05T16:23:24Z

Tangem/Modules/Send/UI/Base/SendCoordinatorView.swift

        }
+        .accentColor(Colors.Text.primary1)


accentColor вроде deprecated. Можно tint использовать

amuraveinik requested review from tureck1y and a team as code owners November 4, 2024 10:45

amuraveinik requested review from Andoran90 and fedorov-d November 4, 2024 10:45

amuraveinik marked this pull request as draft November 4, 2024 10:45

amuraveinik commented Nov 4, 2024

View reviewed changes

Balashov152 requested changes Nov 5, 2024

View reviewed changes

amuraveinik added 4 commits November 5, 2024 16:23

IOS-8357 Add Countries module

5e660b1

IOS-8357 Add Currencies module

9c58c4e

IOS-8357 Add Settings module

81e1e53

IOS-8357 Add routing and renaming

620ca12

amuraveinik force-pushed the feature/IOS-8357_onramp_country_currency_settings branch from e38aa41 to 620ca12 Compare November 5, 2024 15:27

amuraveinik marked this pull request as ready for review November 5, 2024 15:31

amuraveinik requested a review from Balashov152 November 5, 2024 15:31

Balashov152 reviewed Nov 5, 2024

View reviewed changes

IOS-8357 CR fixes

095701b

amuraveinik requested a review from Balashov152 November 5, 2024 19:10

		repository.preferenceCountryPublisher
		.assign(to: &$selectedCountry)

	repository.preferenceCountryPublisher
	.assign(to: &$selectedCountry)
	selectedCountry = repository.preferenceCountry

	let loweccasedSearchText = searchText.lowercased()
	let loweccasedSearchText = searchText.lowercased().trimmed()

IOS-8357 IOS-8158 IOS-8358 [Onramp] Country + Currency + Settings #4155

Are you sure you want to change the base?

IOS-8357 IOS-8158 IOS-8358 [Onramp] Country + Currency + Settings #4155

Conversation

amuraveinik commented Nov 4, 2024 • edited by jira bot Loading

amuraveinik Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Balashov152 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuraveinik commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuraveinik Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuraveinik commented Nov 4, 2024 •

edited by jira bot

Loading

amuraveinik Nov 4, 2024 •

edited

Loading

amuraveinik Nov 5, 2024 •

edited

Loading