Skip to content
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

Introduce AdaptedEnvironmentScreen, for modifying the ViewEnvironment when creating screens #195

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

kyleve
Copy link
Contributor

@kyleve kyleve commented Mar 14, 2023

This mirrors the API allowed on Elements in blueprint, to ease ViewEnvironment modification. For example, you might do something like this:

func render(...) -> Rendering {
    MarketSelectSheetScreen( ... construct your select actions ...)
        .adaptedEnvironment(keyPath: \.isContentAreaEnabled, value: state.isSaving == false)
}

Checklist

  • Unit Tests
  • UI Tests
  • Snapshot Tests (iOS only)
  • I have made corresponding changes to the documentation

@kyleve kyleve requested a review from a team as a code owner March 14, 2023 19:30
WorkflowUI/Sources/Screen/AdaptedEnvironmentScreen.swift Outdated Show resolved Hide resolved
Comment on lines +25 to +40
/// The screen wrapped by this screen.
public var wrapped: Content

/// Takes in a mutable `ViewEnvironment` which can be mutated to add or override values.
public typealias Adapter = (inout ViewEnvironment) -> Void

var adapter: Adapter
Copy link
Contributor

@square-tomb square-tomb Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really matter because this is a value type, but why can't these be let? It doesn't look like we're using their varness in the implementation, tests, or sample usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to just keep things as var unless let is absolutely required (eg, consistency between variables), mostly as a purely stylistic thing.

WorkflowUI/Sources/Screen/AdaptedEnvironmentScreen.swift Outdated Show resolved Hide resolved
WorkflowUI/Sources/Screen/AdaptedEnvironmentScreen.swift Outdated Show resolved Hide resolved
func test_wrapping() {
var environment: ViewEnvironment = .empty

let screen = TestScreen { environment = $0 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the type of this variable is something like:

AdaptedEnvironmentScreen<AdaptedEnvironmentScreen<...<TestScreen>...>>

is allowing nesting like that in the type desirable? in practice would this sort of thing be expected to occur?

there are a few alternatives i can think of for accumulating the environment mutations, but just wondering if it seems worthwhile to explore them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be, yeah. I think that's generally fine; since deeply nesting like this in practice won't really occur. This test is a bit of a degenerate test, designed to simulate many adaptedEnvironments that could be throughout your Screen tree.

there are a few alternatives i can think of for accumulating the environment mutations

We do this in Blueprint, but its a lot easier because the AdaptedEnvironment element there isn't generic around the element type; the generics make this much more difficult.


_ = screen.viewControllerDescription(environment: .empty)

// The inner-most change; the one closest to the screen; should be the value we get.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at first glance, this seems a bit confusing, and almost feels backwards. i can see why it is true and expected (the adapters are applied from 'outside in'), but it makes the example code above seem more confusing. since the outer wrappers that attempt to adapt earlier keys effectively do nothing, does it make sense to allow them to be written? should there be some form of warning produced if clients attempt to do this? as implemented here, this might not be possible to detect, but just something maybe worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this above; but this is basically a somewhat artificial test / a test designed to simulate where these adaptations happen throughout the screen tree; to make sure the "last write wins" is true.

should there be some form of warning produced if clients attempt to do this

I don't think this is really possible; without also including the ViewEnvironmentKey in the generic of the screen; which then also means we'd need to get rid of the keypath and closure-based modifiers.

@kyleve kyleve merged commit 01ebdc7 into main Mar 16, 2023
@kyleve kyleve deleted the kve/adapted-screen branch March 16, 2023 21:56
/// .adaptedEnvironment(keyPath: \.myValue, to: newValue)
/// ```
///
public struct AdaptedEnvironmentScreen<Content: Screen>: Screen {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at #106 from @bencochran, I think I should likely move this conformance to an extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants