Should we create CardContainerModel?

[samreid]

From #284 and related to #283

While investigating this, @marlitas and I observed that there is no model for the order of the cards--it is all handled in the CardContainerNode. So we should investigate better model/view separation for that.

[marlitas]

@samreid, @matthew-blackman, and I chatted about this, and decided that we want to do this correctly. There are a lot of muddled aspects right now, but believe that starting in baby steps is the way to go with this. A good place to start may be with separating out the positionProperty and animation in CardNode out to the model.

This is not a high priority for @samreid before his vacation time.

[marlitas]

Let's double check that the API call here still works as expected after these changes.

[marlitas]

This issue is ready for review. Over to you @matthew-blackman

[matthew-blackman]

Reviewed the code changes and things are looking really good here. Nice work @marlitas! The API call for .getData works great, and it's much easier to get to on the CardContainerModel. Behavior looks good, and these changes fixed #336 and #337 as well. Nice!

I did run into an error when setting a card's cellPositionProperty. It looks like setting it didn't impact the card's position, and then trying to drag a card caused the following:

I haven't looked into what could be causing this yet but it should be fixed before closing this issue.

Lastly, I have a couple questions about PhET-IO instrumentation:

1. Does totalDragDistanceProperty need to be instrumented? We could instead use a boolean for 'showDragIndicator' if we want to be able to customize the drag indicator - any reason to expose the actual drag distance via PhET-iO?
2. In studio, dragIndicationCardProperty behaves in a way I didn't expect. If I click 'Get Value', its phet-io ID is "centerAndVariability.medianScreen.model.cardContainerModel.cards.card1". But if I try to change the last part to "card3" and click 'Set Value', nothing happens. I also tried pasting the 'Get Command' code in the console and nothing happened. But I'm wondering, does this need to be instrumented at all? Having the drag indicator over the leftmost card seems to make sense to me, and I think its visibility would be enough to instrument via PhET-iO. Thoughts @marlitas?
3. Should the cards value be more accessible via studio, even if they are read only? Right now to see a card's value, the user needs to navigate to the linked soccer ball.

Overall this is looking really great and the refactor is a big improvement! Once we clear up a few more things we should be good to go here. Let me know if you have any questions or want to discuss the above further @marlitas.

[marlitas]

I did run into an error when setting a card's cellPositionProperty. It looks like setting it didn't impact the card's position, and then trying to drag a card caused the following:

Mmm good catch. I think the cellPositionProperty should be readonly. I'll update that.

1. I think it needs to be instrumented for state, but perhaps we can make it readonly? I'll double check the state thing though.
2. I also tend to agree that we probably don't need to instrument setting the position of the dragIndicator, but it will probably be good to double check with @catherinecarter before moving forward on that. making it readonly might be enough to solve this, but if we do want to move it around we'll have to look a bit deeper into what's going on.
3. That's definitely a design question... Let's check in with @catherinecarter.

[catherinecarter]

Can you help me understand what the question is? I see a comment about the cellPositionProperty, but then another comment about the dragIndicator.

I agree the cellPositionProperty should be readOnly. Can you help me understand how these two are connected? Thanks!

[marlitas]

Can you help me understand how these two are connected? Thanks!

They are just connected because of a code refactor.

The two big questions for you are:

1. In studio, dragIndicationCardProperty behaves in a way I didn't expect. If I click 'Get Value', its phet-io ID is "centerAndVariability.medianScreen.model.cardContainerModel.cards.card1". But if I try to change the last part to "card3" and click 'Set Value', nothing happens. I also tried pasting the 'Get Command' code in the console and nothing happened. But I'm wondering, does this need to be instrumented at all? Having the drag indicator over the leftmost card seems to make sense to me, and I think its visibility would be enough to instrument via PhET-iO. Thoughts @marlitas?

2. Should the cards value be more accessible via studio, even if they are read only? Right now to see a card's value, the user needs to navigate to the linked soccer ball.

[marlitas]

cellPositionProperty is now readOnly.

[catherinecarter]

I see. Thanks!

I also noticed the Set Value didn't do anything. I was trying to figure out why nothing happened.

1. To add commentary (not sure it's a decision), I can see an instructional designer wanting to put the dragIndicator on a specific card, perhaps to encourage students to notice a particular card, like maybe it's only one out of order. I can also see the indicator being on the left-most card always since it's purpose is to help the user know you can move the cards.

2. I believe you can currently get the value of all the cards as an array via the console, which gives all the values of the cards in the order they currently are (Add a phet-io API call to get the order of the cards in the median screen #283). I'll keep thinking about whether the cards should be more accessible via studio.

I don't think I provided any clarity. I'll keep thinking on it and contribute again soon.

[marlitas]

Does totalDragDistanceProperty need to be instrumented? We could instead use a boolean for 'showDragIndicator' if we want to be able to customize the drag indicator - any reason to expose the actual drag distance via PhET-iO?

Turns out totalDragDistanceProperty is not needed for state. I removed it's instrumentation and all seems to be working well. @matthew-blackman, want to double check it?

[marlitas]

During a check-in we decided:

1. dragIndicationCardProperty should be readOnly. Clients do not need to be able to set it's position.
2. We do not feel like it is needed to add another valueProperty to the card. We could not think of a strong enough client usage case to add this in.

[marlitas]

The above commit addresses all the review comments that are left. I believe this is now ready to close, but please feel free to re-open if there is something else. Thanks @matthew-blackman!