Feat/dark mode webview #274
Conversation
Core/Core/View/Base/Webview/Models/ColorInvertionInjection.swift
Outdated
Show resolved
Hide resolved
forgotvas
dismissed stale reviews from volodymyr-chekyrta and rnr
via
b4eee5a
| public func webView(_ webView: WKWebView, didStartProvisionalNavigation navigation: WKNavigation!) { | ||
| webView.isHidden = true | ||
| } |
There was a problem hiding this comment.
Is it important to hide the webview here? as hiding it here is required webview?isHidded = true at multiple places
There was a problem hiding this comment.
it's important to hide webview on start loading, because inversion script will start to work only when page will be loaded and if not to hide it, then user will see light mode on start and then it will turn into dark mode. So idea is show only dark mode.
There was a problem hiding this comment.
Is it possible to use isLoading property here, so the user will see loading while the inversion is happening?
There was a problem hiding this comment.
no, isLoading property doesn't hide webView it's shows ProgressView on parent view.
There was a problem hiding this comment.
Actually, that's what I'm trying to understand why it's needed, because if it's user-facing and there won't be any loading screen, and we also hide the webview, the view will not look good. If we se the isloading true, the user will see a loading screen and behind the scene the color inversion will happen.
There was a problem hiding this comment.
just to clarify, do you want to set isHidden when we set isLoading? If so then it will work.
There was a problem hiding this comment.
without isHidden:
Simulator.Screen.Recording.-.iPhone.15.-.2024-02-12.at.13.57.16.mp4
with isHidden
Simulator.Screen.Recording.-.iPhone.15.-.2024-02-12.at.13.58.04.mp4
| } | ||
|
|
||
| public func webView(_ webView: WKWebView, didFail navigation: WKNavigation!, withError error: Error) { | ||
| webView.isHidden = false |
There was a problem hiding this comment.
We need to create a task for handling the review failure case. In which we might add a loading error with the reload button. Thoughts?
There was a problem hiding this comment.
@rnr ^^^, @saeedbashir maybe it should be part of #276 (comment)
There was a problem hiding this comment.
I agree it might be separate ticket
| @@ -16,11 +16,12 @@ public enum LessonType: Equatable { | |||
| case discussion(String, String, String) | |||
|
|
|||
| static func from(_ block: CourseBlock, streamingQuality: StreamingQuality) -> Self { | |||
| let mandatoryInjections: [WebviewInjection] = [.inversionCss, .ajaxCallback] | |||
There was a problem hiding this comment.
The name is fine, but how about renaming it to webviewInjections thoughts?
There was a problem hiding this comment.
i'm not sure that it will be correct, idea is to name that variable to indicate that these are the necessary injections for all webviews.
| @@ -47,6 +47,11 @@ public extension WebviewInjection { | |||
| .webviewInjection() | |||
| } | |||
|
|
|||
| static var inversionCss: WebviewInjection { | |||
There was a problem hiding this comment.
@forgotvas Although the name is fine, but what do you think about renaming it to colorInversionCss, for more clear name as inversion can be of any type thoughts?
forgotvas
dismissed stale reviews from volodymyr-chekyrta and rnr
via
2a311a0
PR contains fix for: