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

Scene buttons may not line up with scene reset buttons #348

Closed
KatieWoe opened this issue Jun 5, 2020 · 7 comments
Closed

Scene buttons may not line up with scene reset buttons #348

KatieWoe opened this issue Jun 5, 2020 · 7 comments

Comments

@KatieWoe
Copy link
Contributor

KatieWoe commented Jun 5, 2020

For phetsims/qa#505. Mostly scene in studio, but discovered in multi wrapper.
When you make one of the scene buttons invisible, the others readjust to take its place. However, the reset buttons for those scenes do not move when this happens, meaning the reset buttons are no longer properly lined up the the scene they control.
resetsceneindependent

@samreid
Copy link
Member

samreid commented Jun 10, 2020

This seems good to work on for the next dev version.

@samreid
Copy link
Member

samreid commented Jun 17, 2020

The quick fix would be to disable relayout of that section. That would take 5 minutes and probably be good enough for a dev version.

The better long-term fix would be to reimplement that code around the idea of putting the reset button as part of the radio button components. That was desired for the original design, but we decided against it in order to speed up turnaround time for a dev version.

Therefore, I recommend to commit the quick fix to unblock a dev version. But we should try again to reimplement that code for better long term behavior for a later dev version or RC.

samreid added a commit that referenced this issue Jun 17, 2020
@samreid
Copy link
Member

samreid commented Jun 17, 2020

After the commit, the radio button no longer relayout based on their visibility:

image

Let's call that sufficient for this issue and I'll open a new issue about the long term fix prescribed in the preceding comment. @KatieWoe can you please confirm on master? If it works OK, please reassign to @arouinfar to make sure this is acceptable for a dev version.

@KatieWoe
Copy link
Contributor Author

Looks fixed on master

@KatieWoe KatieWoe assigned samreid and unassigned KatieWoe Jun 17, 2020
@samreid samreid assigned arouinfar and unassigned samreid Jun 17, 2020
@samreid
Copy link
Member

samreid commented Jul 10, 2020

@arouinfar is this acceptable for a dev version?

@arouinfar
Copy link
Contributor

Therefore, I recommend to commit the quick fix to unblock a dev version. But we should try again to reimplement that code for better long term behavior for a later dev version or RC.

@samreid I agree with this 100%. The quick fix is great for the dev version.

@arouinfar arouinfar assigned samreid and unassigned arouinfar Jul 10, 2020
@samreid
Copy link
Member

samreid commented Jul 10, 2020

Thanks, closing. We will continue in #350

@samreid samreid closed this as completed Jul 10, 2020
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

3 participants