Introduce .viewEnvironment(_:_:) modifier#57
Conversation
| var value: Value | ||
| } | ||
|
|
||
| extension View { |
There was a problem hiding this comment.
The fact that attaching viewEnvironment(_:_:) to anything other than a View conforming to WebViewRepresentable results in a compilation error might make extending View feel a bit unnatural.
An alternative approach would be to implement modifiers separately for WebView and ModifiedViewEnvironment, but that would be redundant, so I chose this implementation instead.
I would appreciate any thoughts you may have on this.
There was a problem hiding this comment.
This is a less essential question, but why did you choose the name viewEnvironment instead of webViewEnvironment?
There was a problem hiding this comment.
There might not have been a particularly strong intention behind naming it viewEnvironment. If I had to point out one reason, however, it would be that this modifier can change not just properties belonging to WKWebView, but also those of UIView and UIScrollView. That’s why I chose viewEnvironment instead of webViewEnvironment.
There was a problem hiding this comment.
I initially thought webViewEnvironment was appropriate since there’s an inheritance relationship, but I understand your reasoning as well. I’m now okay with either naming.
| .allowsBackForwardNavigationGestures(true) | ||
| .allowsLinkPreview(false) | ||
| .refreshable() | ||
| .viewEnvironment(\.customUserAgent, "Customized User Agent") |
There was a problem hiding this comment.
I made some modifications to the example app to write UI tests for viewEnvironment(_:_). However, I feel that having deprecated modifiers in the example app might be confusing for readers.
What do you think?
There was a problem hiding this comment.
@b1ackturtle
How about commenting that it is a workaround example?
There was a problem hiding this comment.
Do we need to test for deprecated API? (Of course it is better to have it.)
It might be good to have some explanation in the README that there is a temporary workaround.
Also, it was not the UserAgent that triggered the need for this modifier.
Since our original intention was to have the UserAgent specified in WKWebViewConfiguration, it may be more appropriate to present a different use case.
If you want additional specifications for webview-debugger, let me know. I will add it right away.
There was a problem hiding this comment.
How about commenting that it is a workaround example?
I think this workaround will function properly.
Do we need to test for deprecated API?
Since it’s deprecated, it might be acceptable not to write tests for it.
it was not the UserAgent that triggered the need for this modifier.
You made a valid point.
I prioritized something that would simply work and wrote a UI test using a custom user agent.
However, that’s not really optimal.
First, we need to decide whether we want to guarantee its behavior with tests or not.
Personally, since this modifier isn’t intended to be fully supported, I’m starting to think we might be able to do without it.
What do you think? @elmetal @Kyome22
There was a problem hiding this comment.
In my opinion, this is a workaround rather than a deprecation, so I don’t think marking it as deprecated would be accurate. That said, we’re currently using deprecated to indicate that we don’t intend to provide full support, but I’m open to changing it if there’s a better way to express that.
There was a problem hiding this comment.
I tried removing deprecated and building the document, but the message no longer appears.
- @available(*, deprecated, message: "This ViewModifier ... GitHub repository.")
+ @available(*, message: "This ViewModifier ... GitHub repository.")Is there another way to make it explicit that this API is a workaround? I don't know.
There was a problem hiding this comment.
@Kyome22
I don’t know how to solve this either. We might have to accept the limitations in expressiveness and just mark it as deprecated.
There was a problem hiding this comment.
I, too, feel it can only be described as deprecated.
Given that it's marked as deprecated, do you think this API should be tested to ensure it functions correctly?
There was a problem hiding this comment.
If it were literally deprecated, we wouldn’t need to test it—but since this is a matter of expressiveness, I think it’s worth keeping the tests.
| public func viewEnvironment<Value>( | ||
| _ keyPath: ReferenceWritableKeyPath<WKWebView, Value>, | ||
| _ value: Value | ||
| ) -> ModifiedViewEnvironment<Self, Value> { |
There was a problem hiding this comment.
The return type name of ViewModifier does not look like View, so there is a sense of discomfort when using it.
For example, ModifiedEnvironmentView or ModifiedEnvironmentWebView might be easier to understand.
There was a problem hiding this comment.
It seems like some View for the declaration here would be more appropriate.
In case we decide not to use some View, ModifiedViewEnvironmentContent might be a good fit.
It aligns well with SwiftUI’s naming conventions like ModifiedContent, and clearly expresses that the content has been altered by changes to the environment.
There was a problem hiding this comment.
As @elmetal also mentioned, I named ModifiedViewEnvironment with ModifiedContent in mind.
However, since it seems a bit unclear, I’m planning to rename it to ModifiedViewEnvironmentContent.
3 participants
No description provided.