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

What is happening in new DynamicProperty( this.leftBarrierViewPointProperty ).lazyLink( resizeBarrier );? #26

Closed
samreid opened this issue Aug 10, 2021 · 6 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Aug 10, 2021

During code review #5, I noticed:

new DynamicProperty( this.leftBarrierViewPointProperty ).lazyLink( resizeBarrier );

How is this different from

this.leftBarrierViewPointProperty.lazyLink( resizeBarrier );

I saw a total of 3 DynamicProperty instantiations this applies to.

@jonathanolson
Copy link
Contributor

leftBarrierViewPointProperty/rightBarrierViewPointProperty are Properties of Properties, and we need to also listen to the secondary Property for updates. Should those fields be renamed to something with ...PropertyProperty to better indicate that?

@samreid
Copy link
Member Author

samreid commented Aug 10, 2021

I don't understand why rightBarrierViewPointProperty should be a Property<Property>. Isn't it a constant value depending on the screen? For instance, BuoyancyExploreScreenView always has:

    this.rightBarrierViewPointProperty.value = new DerivedProperty( [ this.rightBox.boundsProperty, this.visibleBoundsProperty ], ( boxBounds, visibleBounds ) => {
      return new Vector2( boxBounds.left, visibleBounds.centerY );
    } )

Why is this designed to be dynamic?

@samreid samreid assigned jonathanolson and unassigned samreid Aug 10, 2021
@jonathanolson
Copy link
Contributor

Why is this designed to be dynamic?

It's easier/simpler to design it that way, in my opinion. No fragility of "when after super() have we properly setup the Property so that NOW we can start to listen to it". Is there a better way?

@jonathanolson
Copy link
Contributor

Isn't it a constant value depending on the screen?

It does eventually end up pointing to a specific Property depending on the screen, yes.

@samreid
Copy link
Member Author

samreid commented Aug 10, 2021

I noticed some places use value.value and some use DynamicProperty. Are all of those usages appropriate, or should some switch? For me and perhaps other developers, DynamicProperty.link may be more opaque than using value.value.

@samreid samreid assigned jonathanolson and unassigned samreid Aug 10, 2021
samreid added a commit that referenced this issue Aug 10, 2021
@samreid
Copy link
Member Author

samreid commented Aug 10, 2021

@jonathanolson and I discussed this, and thought the documentation was the best way to address this, closing.

@samreid samreid closed this as completed Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants