-
Notifications
You must be signed in to change notification settings - Fork 46
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
Pass context with environment to backingViewDescription #231
Conversation
@@ -61,6 +61,14 @@ public protocol Element { | |||
/// `subtreeExtent` will be nil if there are no children. | |||
/// | |||
/// - Returns: An optional `ViewDescription`. | |||
func backingViewDescription(bounds: CGRect, subtreeExtent: CGRect?) -> ViewDescription? | |||
func backingViewDescription(bounds: CGRect, subtreeExtent: CGRect?, with context: ElementContext) -> ViewDescription? |
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.
I assume just adding the context here so refactoring is easier? Would be nice to make this one param...
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.
(Looks like there's 134 of these in SPOS)
~/Desktop/Development/Register1(kve/bump-listable-jun-15-2021):0 git grep "func backingViewDescription(bounds:" | wc -l
134
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.
(Looks like about 120 of these return views; the rest return nil.)
@@ -413,6 +415,7 @@ private struct LazyStorage: ContentStorage { | |||
let node = LayoutResultNode( | |||
element: child, | |||
layoutAttributes: childAttributes, | |||
environment: environment, |
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.
Oh smart! I guess adding this to the layout result does get us the right environment; nice!!
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.
Less "smart" and more me just adding the environment to the backingViewDescription
and finding myself here, where there was already an environment passed in 😂 but yeah, glad it worked it out like that!
@@ -36,6 +36,13 @@ public final class BlueprintView: UIView { | |||
|
|||
private let rootController: NativeViewController | |||
|
|||
public var environment: Environment { |
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.
Ah ha, yay! I'd been wanting this for a while.
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.
Yeah me too - added a test as well
4066300
to
e5b4510
Compare
e5b4510
to
56864a5
Compare
/// | ||
/// - Returns: An optional `ViewDescription`. | ||
func backingViewDescription(bounds: CGRect, subtreeExtent: CGRect?) -> ViewDescription? | ||
func backingViewDescription(in context: ElementContext) -> ViewDescription? |
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.
You're going to hate me (sorry); what do we think about:
func viewDescription(with context: ElementContext) -> ViewDescription?
Also, do we want ElementContext
; or should it be ViewDescriptionContext
or something?
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.
Both changes are fine with me! I had with
originally but in
seemed kinda nice with the one param? idk, fine with with
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.
(I think ElementContext
is fine; just wondering if we'll ever want that name for something else)
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.
Already did ViewDescriptionContext
56864a5
to
db9908a
Compare
Adds an
ElementContext
object for passing information tobackingViewDescription
, and adds the environment to the context (along with the existingbounds
andsubtreeExtent
). This allows view-backed elements to access the environment they're rendered in.