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

[fix]: make WorkflowHostingController background color clear #220

Conversation

n8chur
Copy link
Collaborator

@n8chur n8chur commented May 31, 2023

Note: This PR is targeting the feature/viewenvironmentui branch and depends on #218.

Overview

This PR is part of the larger effort to add support for automatic ViewEnvironment bridging between UIKit propagation and WorkflowUI propagation within WorkflowUI.

See the associated proposal for more information.

While this change may seem unrelated to the view environment bridging work, it was added to this feature branch since it allows replacement of DescribedViewController usages with WorkflowHostingControllers without loading the view earlier than you may have needed to previously. WorkflowHostingController rendering a single screen is one way to achieve automatic bridging of the ViewEnvironment from a vanilla propagation path to a Screen which can be useful in contexts like snapshot testing harnesses. While we may want to provide an automatic bridging container specific to Screen's, this work is currently considered out of scope for this feature branch (although that's open to reconsideration)—see the this section in the proposal for more information on this topic.

DescribedViewController's backgroundColor is .clear, which is an intuitive value when considering that it should appear identical to the content it is wrapping—it shouldn't place an opaque .white background underneath. I'd argue the same applies to WorkflowHostingController.

Today, if you want to set the WorkflowHostingController's backgroundColor to .clear, you'd need to load the view first (workflowHostingController.view.backgroundColor = .clear)—there is no way override the background color otherwise. Loading the view early like this can be problematic in some cases, e.g. when attempting to utilize a WorkflowHostingController in snapshot testing infrastructure where you may want to build up a UIViewController hierarchy before loading the view at all in case it references values in the vanilla ViewEnvironment propagation path which need to be mutated above it.

Square Integration PRs

https://github.com/squareup/market/pull/6286
https://github.com/squareup/ios-register/pull/86301

Checklist

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

@n8chur n8chur changed the title [fix]: make WorkflowHostingController background color clear [WIP DNR] [fix]: make WorkflowHostingController background color clear May 31, 2023
@n8chur n8chur changed the title [WIP DNR] [fix]: make WorkflowHostingController background color clear [fix]: make WorkflowHostingController background color clear Jun 1, 2023
@n8chur n8chur marked this pull request as ready for review June 1, 2023 16:34
@n8chur n8chur requested a review from a team as a code owner June 1, 2023 16:34
@n8chur n8chur requested a review from a team June 1, 2023 16:34
Copy link
Contributor

@square-tomb square-tomb left a comment

Choose a reason for hiding this comment

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

Is it possible that some of the sample apps in Samples will become illegible with this change?

view.backgroundColor = .white
view.backgroundColor = .clear
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should have been .backgroundColor in the first place 🤔

Copy link
Member

@robmaceachern robmaceachern left a comment

Choose a reason for hiding this comment

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

@n8chur
Copy link
Collaborator Author

n8chur commented Jun 1, 2023

Is it possible that some of the sample apps in Samples will become illegible with this change?

It's possible! I'll browse through them and double check.

  • Validate that sample app backgrounds still look good

Copy link
Collaborator

@watt watt left a comment

Choose a reason for hiding this comment

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

Not sure if you're keeping track elsewhere, but make sure this gets into the changelog!

@n8chur
Copy link
Collaborator Author

n8chur commented Jun 7, 2023

make sure this gets into the changelog!

I don't think the changelog within the repo is maintained, is it? It looks like it's generated for the release description manually based on PRs.

@n8chur n8chur force-pushed the westin/viewenvironment-clear-background-whc branch from 965f764 to e728f84 Compare June 8, 2023 16:41
Base automatically changed from westin/viewenvironment-object-nodes-only to feature/viewenvironmentui June 8, 2023 16:51
@n8chur n8chur force-pushed the westin/viewenvironment-clear-background-whc branch from e728f84 to 50293db Compare June 8, 2023 16:51
@n8chur n8chur merged commit ef0aaee into feature/viewenvironmentui Jun 8, 2023
@n8chur n8chur deleted the westin/viewenvironment-clear-background-whc branch June 8, 2023 17:35
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.

5 participants