-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Program Screen Error Handling #448
Changes from 4 commits
aaf0cee
d37230f
f6472c2
25e374f
e766263
8002284
954d41b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
// | ||
// FullScreenErrorView.swift | ||
// Course | ||
// | ||
// Created by Shafqat Muneer on 5/14/24. | ||
// | ||
|
||
import SwiftUI | ||
import Theme | ||
|
||
public struct FullScreenErrorView: View { | ||
|
||
public enum ErrorType { | ||
case noInternet | ||
case noInternetWithReload | ||
case generic | ||
} | ||
|
||
private let errorType: ErrorType | ||
private var reloadAction: () -> Void = {} | ||
|
||
public init( | ||
type: ErrorType | ||
) { | ||
self.errorType = type | ||
} | ||
|
||
public init( | ||
type: ErrorType, | ||
reloadAction: @escaping () -> Void | ||
) { | ||
self.errorType = type | ||
self.reloadAction = reloadAction | ||
} | ||
|
||
public var body: some View { | ||
GeometryReader { proxy in | ||
VStack(spacing: 28) { | ||
Spacer() | ||
switch errorType { | ||
case .noInternet, .noInternetWithReload: | ||
CoreAssets.noWifi.swiftUIImage | ||
.renderingMode(.template) | ||
.foregroundStyle(Color.primary) | ||
.scaledToFit() | ||
|
||
Text(CoreLocalization.Error.Internet.noInternetTitle) | ||
.font(Theme.Fonts.titleLarge) | ||
.foregroundColor(Theme.Colors.textPrimary) | ||
|
||
Text(CoreLocalization.Error.Internet.noInternetDescription) | ||
.font(Theme.Fonts.bodyLarge) | ||
.foregroundColor(Theme.Colors.textPrimary) | ||
.multilineTextAlignment(.center) | ||
.padding(.horizontal, 50) | ||
case .generic: | ||
CoreAssets.notAvaliable.swiftUIImage | ||
.renderingMode(.template) | ||
.foregroundStyle(Color.primary) | ||
.scaledToFit() | ||
|
||
Text(CoreLocalization.View.Snackbar.tryAgainBtn) | ||
.font(Theme.Fonts.titleLarge) | ||
.foregroundColor(Theme.Colors.textPrimary) | ||
|
||
Text(CoreLocalization.Error.unknownError) | ||
.font(Theme.Fonts.bodyLarge) | ||
.foregroundColor(Theme.Colors.textPrimary) | ||
.multilineTextAlignment(.center) | ||
.padding(.horizontal, 50) | ||
} | ||
|
||
if errorType != .noInternet { | ||
UnitButtonView(type: .reload, action: { | ||
self.reloadAction() | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. formatting
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done 🎉 |
||
} | ||
Spacer() | ||
} | ||
.frame(maxWidth: .infinity, maxHeight: proxy.size.height) | ||
.background( | ||
Theme.Colors.background | ||
) | ||
} | ||
} | ||
} | ||
|
||
#if DEBUG | ||
struct FullScreenErrorView_Previews: PreviewProvider { | ||
static var previews: some View { | ||
FullScreenErrorView(type: .noInternetWithReload) | ||
} | ||
} | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ public protocol WebViewNavigationDelegate: AnyObject { | |
shouldLoad request: URLRequest, | ||
navigationAction: WKNavigationAction | ||
) async -> Bool | ||
|
||
func showWebViewError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It nice and can work with passing the error, but how about having a more detailed method like
Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In our use case, displaying specific error details is not required. We are currently only showing internet error or generic error. Therefore, detailed information is not required at this time. We can reconsider this approach if there arises a need for more detailed error messages in the future. |
||
} | ||
|
||
public struct WebView: UIViewRepresentable { | ||
|
@@ -39,17 +41,20 @@ public struct WebView: UIViewRepresentable { | |
var webViewNavDelegate: WebViewNavigationDelegate? | ||
|
||
var refreshCookies: () async -> Void | ||
var webViewType: String? | ||
|
||
public init( | ||
viewModel: ViewModel, | ||
isLoading: Binding<Bool>, | ||
refreshCookies: @escaping () async -> Void, | ||
navigationDelegate: WebViewNavigationDelegate? = nil | ||
navigationDelegate: WebViewNavigationDelegate? = nil, | ||
webViewType: String? = nil | ||
) { | ||
self.viewModel = viewModel | ||
self._isLoading = isLoading | ||
self.refreshCookies = refreshCookies | ||
self.webViewNavDelegate = navigationDelegate | ||
self.webViewType = webViewType | ||
} | ||
|
||
public class Coordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WKScriptMessageHandler { | ||
|
@@ -70,6 +75,10 @@ public struct WebView: UIViewRepresentable { | |
|
||
public func webView(_ webView: WKWebView, didFail navigation: WKNavigation!, withError error: Error) { | ||
webView.isHidden = false | ||
DispatchQueue.main.async { | ||
self.parent.isLoading = false | ||
} | ||
parent.webViewNavDelegate?.showWebViewError() | ||
} | ||
|
||
public func webView( | ||
|
@@ -78,6 +87,10 @@ public struct WebView: UIViewRepresentable { | |
withError error: Error | ||
) { | ||
webView.isHidden = false | ||
DispatchQueue.main.async { | ||
self.parent.isLoading = false | ||
} | ||
parent.webViewNavDelegate?.showWebViewError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about
and remove DispatchQueue.main from the showWebViewError function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, it's more appropriate I believe. Thanks |
||
} | ||
|
||
public func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) { | ||
|
@@ -172,7 +185,7 @@ public struct WebView: UIViewRepresentable { | |
|
||
private func addObservers() { | ||
cancellables.removeAll() | ||
NotificationCenter.default.publisher(for: .webviewReloadNotification, object: nil) | ||
NotificationCenter.default.publisher(for: Notification.Name(parent.webViewType ?? ""), object: nil) | ||
.sink { [weak self] _ in | ||
self?.reload() | ||
} | ||
|
@@ -188,8 +201,16 @@ public struct WebView: UIViewRepresentable { | |
fileprivate var webview: WKWebView? | ||
|
||
@objc private func reload() { | ||
parent.isLoading = true | ||
webview?.reload() | ||
DispatchQueue.main.async { | ||
self.parent.isLoading = true | ||
} | ||
if webview?.url?.absoluteString.isEmpty ?? true, | ||
let url = URL(string: parent.viewModel.url) { | ||
let request = URLRequest(url: url) | ||
webview?.load(request) | ||
} else { | ||
webview?.reload() | ||
} | ||
} | ||
|
||
public func userContentController( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,7 +311,6 @@ | |
02B6B3B528E1D10700232911 /* Domain */, | ||
02EAE2CA28E1F0A700529644 /* Presentation */, | ||
97EA4D822B84EFA900663F58 /* Managers */, | ||
97CA95212B875EA200A9EDEA /* Views */, | ||
02B6B3B428E1C49400232911 /* Localizable.strings */, | ||
); | ||
path = Course; | ||
|
@@ -526,13 +525,20 @@ | |
path = Mock; | ||
sourceTree = "<group>"; | ||
}; | ||
97CA95212B875EA200A9EDEA /* Views */ = { | ||
9784D4752BF39EEF00AFEFFF /* CalendarSyncProgressView */ = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to update this? Asking because it doesn't seem relevant to the work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated location from:
To:
I found that we already have subviews folder and it seems more appropriate to place it there. |
||
isa = PBXGroup; | ||
children = ( | ||
97C99C352B9A08FE004EEDE2 /* CalendarSyncProgressView.swift */, | ||
); | ||
path = CalendarSyncProgressView; | ||
sourceTree = "<group>"; | ||
}; | ||
9784D4762BF39EFD00AFEFFF /* DatesSuccessView */ = { | ||
isa = PBXGroup; | ||
children = ( | ||
97CA95242B875EE200A9EDEA /* DatesSuccessView.swift */, | ||
); | ||
path = Views; | ||
path = DatesSuccessView; | ||
sourceTree = "<group>"; | ||
}; | ||
97EA4D822B84EFA900663F58 /* Managers */ = { | ||
|
@@ -592,6 +598,8 @@ | |
BAD9CA482B2C88D500DE790A /* Subviews */ = { | ||
isa = PBXGroup; | ||
children = ( | ||
9784D4762BF39EFD00AFEFFF /* DatesSuccessView */, | ||
9784D4752BF39EEF00AFEFFF /* CalendarSyncProgressView */, | ||
02D4FC2C2BBD7C7500C47748 /* MessageSectionView */, | ||
BAC0E0D92B32F0A2006B68A9 /* CourseVideoDownloadBarView */, | ||
BA58CF622B471047005B102E /* VideoDownloadQualityBarView */, | ||
|
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.
Could we generalize it for the future and call it just
action
?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.
Yes, it's updated with a more generic name.