-
Notifications
You must be signed in to change notification settings - Fork 121
Store creation M2 - domain selection: empty/error/loading states, design updates #8092
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
a597160
Use navigation bar large title for store creation domain selector.
jaclync a154767
Handle error by showing an error message. Improve landscape layout by…
jaclync c127118
Add domain search placeholder image asset.
jaclync c4e4da7
Show placeholder image when search query is empty.
jaclync 7c84612
Add customizations to `SearchHeader` component for `DomainSelectorVie…
jaclync 922a178
Comment and access control updates.
jaclync d6d8b86
Update unit tests to wait for reactive property updates.
jaclync 1cffbd7
Update domain search placeholder asset - still might need a dark mode…
jaclync 0b0d34c
Merge branch 'trunk' into feat/8045-domain-selector-updates
jaclync 42d811f
Replace `DomainSelectorViewModel.placeholderImage` with `searchTerm` …
jaclync File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import Combine | ||
| import SwiftUI | ||
| import Yosemite | ||
| import enum Networking.DotcomError | ||
|
|
||
| /// View model for `DomainSelectorView`. | ||
| final class DomainSelectorViewModel: ObservableObject { | ||
|
|
@@ -11,30 +12,55 @@ final class DomainSelectorViewModel: ObservableObject { | |
| /// Domain names after domain suggestions are loaded remotely. | ||
| @Published private(set) var domains: [String] = [] | ||
|
|
||
| /// Error message from loading domain suggestions. | ||
| @Published private(set) var errorMessage: String? | ||
|
|
||
| /// Whether domain suggestions are being loaded. | ||
| @Published private(set) var isLoadingDomainSuggestions: Bool = false | ||
|
|
||
| /// Placeholder image is set when the search term is empty. Otherwise, the placeholder image is `nil`. | ||
| @Published private(set) var placeholderImage: UIImage? | ||
|
||
|
|
||
| /// Subscription for search query changes for domain search. | ||
| private var searchQuerySubscription: AnyCancellable? | ||
|
|
||
| private let stores: StoresManager | ||
| private let debounceDuration: Double | ||
|
|
||
| init(stores: StoresManager = ServiceLocator.stores, | ||
| init(initialSearchTerm: String = "", | ||
| stores: StoresManager = ServiceLocator.stores, | ||
| debounceDuration: Double = Constants.fieldDebounceDuration) { | ||
| self.stores = stores | ||
| self.debounceDuration = debounceDuration | ||
|
|
||
| // Sets the initial search term after related subscriptions are set up | ||
| // so that the initial value is always emitted. | ||
| // In `observeDomainQuery`, `share()` transforms the publisher to `PassthroughSubject` | ||
| // and thus the initial value isn't emitted in `observeDomainQuery` until setting the value afterward. | ||
| observeDomainQuery() | ||
| self.searchTerm = initialSearchTerm | ||
| } | ||
| } | ||
|
|
||
| private extension DomainSelectorViewModel { | ||
| func observeDomainQuery() { | ||
| searchQuerySubscription = $searchTerm | ||
| let searchTermPublisher = $searchTerm | ||
| .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } | ||
| .debounce(for: .seconds(debounceDuration), scheduler: DispatchQueue.main) | ||
| .share() | ||
|
|
||
| searchQuerySubscription = searchTermPublisher | ||
| .filter { $0.isNotEmpty } | ||
| .removeDuplicates() | ||
| .debounce(for: .seconds(debounceDuration), scheduler: DispatchQueue.main) | ||
| .sink { [weak self] searchTerm in | ||
| guard let self = self else { return } | ||
| Task { @MainActor in | ||
| self.errorMessage = nil | ||
| self.placeholderImage = nil | ||
| self.isLoadingDomainSuggestions = true | ||
| let result = await self.loadFreeDomainSuggestions(query: searchTerm) | ||
| self.isLoadingDomainSuggestions = false | ||
|
|
||
| switch result { | ||
| case .success(let suggestions): | ||
| self.handleFreeDomainSuggestions(suggestions, query: searchTerm) | ||
|
|
@@ -43,6 +69,12 @@ private extension DomainSelectorViewModel { | |
| } | ||
| } | ||
| } | ||
|
|
||
| searchTermPublisher | ||
| .map { | ||
| $0.isEmpty ? UIImage.domainSearchPlaceholderImage: nil | ||
| } | ||
| .assign(to: &$placeholderImage) | ||
| } | ||
|
|
||
| @MainActor | ||
|
|
@@ -66,8 +98,13 @@ private extension DomainSelectorViewModel { | |
|
|
||
| @MainActor | ||
| func handleError(_ error: Error) { | ||
| // TODO-8045: error handling - maybe show an error message. | ||
| DDLogError("Cannot load domain suggestions for \(searchTerm)") | ||
| if let dotcomError = error as? DotcomError, | ||
| case let .unknown(_, message) = dotcomError { | ||
| errorMessage = message | ||
| } else { | ||
| errorMessage = Localization.defaultErrorMessage | ||
| } | ||
| DDLogError("Cannot load domain suggestions for \(searchTerm): \(error)") | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -76,3 +113,11 @@ private extension DomainSelectorViewModel { | |
| static let fieldDebounceDuration = 0.3 | ||
| } | ||
| } | ||
|
|
||
| extension DomainSelectorViewModel { | ||
| enum Localization { | ||
| static let defaultErrorMessage = | ||
| NSLocalizedString("Please try another query.", | ||
| comment: "Default message when there is an unexpected error loading domain suggestions on the domain selector.") | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simply decide visibility of the placeholder image based on
searchTerm. That way we won't need to update the image in the view model. We can also avoid the else-if blocks by usingrenderredIf- the resultsLazyVStackwill not be rendered if the domain list is empty so there's no need to put it in theelseblock I believe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestions, I forgot about
renderedIf! The if/else style ensures that only one conditional UI is shown at a time. I tested the suggested changes, and I'll have to change the timing when errors are reset and maybe more to make sure only one UI is shown at the same time. I think I'll keep the if/else style for now, but I replaced theviewModel.placeholderImagewithviewModel.searchTerm.isEmptyin 42d811f thanks to your suggestion!