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

Move forward/backward should change the visible ordering in the combobox #325

Closed
Tracked by #901 ...
Nancy-Salpepi opened this issue Feb 7, 2023 · 11 comments
Closed
Tracked by #901 ...

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air (m1 chip)

Operating System
13.1

Browser
safari 16.2

Problem description
For phetsims/qa#894, if I removed the two solutes (Co(NO3)2 and CoCl2) listed in the combobox after "Drink Mix," and then try to move "Drink Mix" down in the order, I have to press the down arrow three times before I see a change in the combobox order.

@arouinfar said that this issue was addressed in pH scale but is part of common code. phetsims/ph-scale#266

Visuals

comboboxordering.mp4
@pixelzoom
Copy link
Contributor

I'm guessing that this is a bug in ComboBoxListbox keydown handler. Disappointing that this doesn't work, given that this was a primary reason for reworking ComboBox. I'm going to defer to @samreid for fixing this.

@pixelzoom pixelzoom removed their assignment Feb 7, 2023
@samreid
Copy link
Member

samreid commented Feb 7, 2023

I imagine this will be fixed by cherry picking phetsims/scenery@6fe6985

@samreid samreid removed their assignment Feb 7, 2023
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 8, 2023

@samreid That cherry-pick you're referring to (phetsims/scenery@6fe6985) was committed on 1/11/2023. Why is it not in 1.7.0-rc.2 that was created on 2/6/2023? This suggests that there's still a problem with the 1.7 release branch. No cherry-pick should be necessary.

@samreid
Copy link
Member

samreid commented Feb 8, 2023

I ran

cd beers-law-lab
git checkout 1.7
grunt checkout-shas

Then I checked the history for scenery, and saw this:

image

Note there are no commits from Jan 11, but plenty of commits afterwards.

Checking the history from master, I see the Jan 11 commit and the other nearby commits from @jessegreenberg:

image

So the branch is somehow missing this commit. How is this possible? What else is it missing?

@samreid
Copy link
Member

samreid commented Feb 8, 2023

Oh wait, the dot for the Jan 11 commit is in the branch--you can see the dot is on the left. And it was not merged until Feb 3. So likely we just don't understand why phetsims/scenery@6fe6985 says master.

@pixelzoom OK to relabel ready-for-cherry-pick?

@pixelzoom
Copy link
Contributor

I don't understand. This issue was reported yesterday 2/7, and is therefore for 1.7.0-rc.2 -- after the shas in the 1.7 branch were fixed in #327. The sha that you're proposing to cherry-pick is dated 1/11/23. How can that commit not be in the 1.7 release, and why do we think it's appropriate to cherry-pick it?

@pixelzoom
Copy link
Contributor

Discussed with @samreid on Zoom. Commit phetsims/scenery@6fe6985 was done in a branch 'carousel-dynamic', and it was not merged to master until after the 1.7 release branch was created. So yes, it needs to be cherry-picked. Labeling as such.

@samreid
Copy link
Member

samreid commented Feb 9, 2023

@pixelzoom would you like me to cherry pick this commit? And should I do that right away?

@samreid samreid assigned pixelzoom and unassigned samreid Feb 9, 2023
@pixelzoom
Copy link
Contributor

I have a couple of other things to cherry-pick today, so I'll handle it.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 9, 2023

Cherry-picks completed for beers-law-lab 1.7 anf concentration 1.7 in the above commits.

To verify in the next RC, for both beers-law-lab and concentration:

@stemilymill
Copy link

looks good in beers law lab and concentration

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

4 participants