-
Notifications
You must be signed in to change notification settings - Fork 0
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
When objects are combined their drag handles sometimes appear behind other cards #44
Comments
This is fixed, but I don't fully understand why I needed two If it looks good to @chrisklus feel free to close, but would definitely appreciate a short explanation to fill in my logic gap. Thanks! |
Thanks @marlitas! I have yet to dig in and review but just noticed this is maybe causing a bug on the compare screen where you can't select different object types after combining two counting objects. |
Should this be "blocks-sim-publication" (only blocks publication for this sim), or "blocks-publication" (blocks publication for all sims)? Based on the remarks above, it seems sim-specific, so I will update the label. Please correct me as necessary. |
Thanks @samreid, that's correct. Must have missed when trying to add status:blocks-sim-publication. |
@marlitas and I were pairing a ways back on this issue. We found two cases that needed to be fixed: (1) layering when dropping a card that combines with another (and the result can end up behind a neighbor), and (2) serializing/deserializing the linked play area. For the linked play area case, we came up with an approach that tracked the z-order of the cards in the model. These would listen to view position and then update their model to match. When time to serialize/deserialize, they would use their model Property to recreate themselves in the right order. While this would work, we thought it was weird to have a model Property listening to a view position. So I consulted with @jonathanolson to see what he thought, and he recommended changing this pattern to have the model z-index be the ground truth, and the view reorder itself based on the model. This would mean no longer using For the dropping case, I still need to investigate to see if this is an easy fix. The current fix that is solving that is in a bit too general of a place (in |
For phetsims/qa#917, would you say this is the same issue, or a new one? |
Related to #44 (comment), overlapping.10.frames.mp4 |
@marlitas and I reviewed the state of this issue together. We first discussed how (2) can't be fixed with a model-driven approach as described in #44 (comment). Then we mentioned that we need to look back into the best way to fix (1). Then, we talked about the problems that @KatieWoe and @Nancy-Salpepi demonstrated on the Lab Screen in their comments above (let's label this one (3). We think that they are the same issue, and will be a separate fix from (1) but are conceptually the same as (1) because the "receiving" node (either the counting object that is being added to or the ten frame that is being added to) is not moving to front when something is dropped on it. Here we go! |
… move DraggableTenFrameNodes to the front when they get a countingObject, see #44
In the above commit, @marlitas and I took care of (3). Still to do for this issue: (1). |
@zepumph @samreid and I also figured out a way to fix (2)! We did not change the z-index ordering to be powered by the model, but instead moved the serialization function of the countingObjects to the view. This allowed us to capture the state of the z-indexes without breaking model -> view ordering. Thanks! |
I believe all z-index problems for this issue have been fixed. @Nancy-Salpepi or @KatieWoe can you please test these fixes on phettest? Problems 1 and 2 two are briefly described in #44 (comment) and 3 was labeled in #44 (comment). It might be easiest to discuss 2 or any clarifications needed over Zoom, let me know (also, 2 is just Number Play). Thanks! |
@chrisklus 1 and 3 look good in both Number Play and Number Compare. Let me know on slack when we can zoom to discuss 2. 🙂 |
@Nancy-Salpepi thanks! can you meet around 2:30 MT today? |
@chrisklus I can! Then me know where 🙂 |
Awesome thanks! Will Slack you |
@marlitas and I found a bug from changes to fix (3), where when you click the return button, the removed object immediately goes behind the tenFrame instead of staying in front as it goes to its home. We fixed this in the above commit. |
We created yet another bug from the last bug fix above that we just committed a fix for and we really hope we didn't make anymore problems. We think the ten frames could use a nice little re-test after @Nancy-Salpepi and I meet to make sure all is good after all these changes. |
@Nancy-Salpepi and I are meeting about this at 9:30 tomorrow |
(1) still looks good to me. (2) looks good as well. (3) looks good except for this phetsims/number-compare#30 (comment), but I don't think that is something that will be changed. Closing. 🤞🏻I didn't miss anything. |
The z-order of a combined object needs to be pushed to the front on drag ended. This is probably related to the object being dropped into, not the object being dragged.
![Screenshot 2023-01-31 at 3 52 45 PM](https://user-images.githubusercontent.com/42476338/215910320-57db93a4-967e-4edb-ae4a-6065c9c576d2.png)
This is seen in both NumberCompare and NumberPlay
The text was updated successfully, but these errors were encountered: