phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Replace HSeparatorDeprecated and VSeparatorDeprecated #805

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Replace HSeparatorDeprecated and VSeparatorDeprecated with HSeparator and VSeparator respectively. They should NOT be instrumented for PhET-iO (see https://github.com/phetsims/scenery/issues/1480). Then delete the deprecated classes.

HSeparatorDeprecated occurrences:

VSeparatorDeprecated occurrences:

pixelzoom commented 1 year ago

Done with my occurrences, unassigning.

jbphet commented 1 year ago

I updated the checkbox for build-a-nucleus, since it was assigned to me but I'm not working on that sim. I also updated the assignees of the issue.

samreid commented 1 year ago

Still remaining, clustered by devs:

jbphet commented 1 year ago

I took a shot at sun.ComboBoxButton, but ran into problems. It uses a GridBox layout, and these layouts are apparently incompatible with the new VSeparator classes. I tried using an HBox for the layout instead, but it didn't like the default left align options, which is somewhat understandable, but we really do need some way to position the choices in the box on the left side. I spoke with @jonathanolson about all of this, and he said that there were several ways to potentially approach the problem, and suggested not using a VSeparator at all in this case. He suggested I assign it back to him for a look. So, sorry I was unable to resolve it. Assigning back to @jonathanolson.

samreid commented 1 year ago

Nov 17 2022 Dev Meeting

@jonathanolson: When we need to expand cells and align things, GridBox doesn't support that. So it could be a multi-day issue to get a good long-term solution. But would be better to have more layout work support in advance. But I'm not convinced ComboBoxButton should be using a separator. That seems fragile and should be done with something else. @pixelzoom: Just a Line? @jonathanolson: Yes. @kathy-phet: Can @jonathanolson prioritize this after other urgent maintenance releases are done? @marlitas: This seems like something I could help on, I can take a look and collaborate with @jonathanolson when he has time @kathy-phet great! @pixelzoom: @marlitas please move that part of the work to a new issue in sun.

marlitas commented 1 year ago

Unassigning myself and @Luisav1 from this issue.

marlitas commented 1 year ago

@jonathanolson and @zepumph, would you like me to handle the deprecated usages that are assigned to you?

jonathanolson commented 1 year ago

@jonathanolson and @zepumph, would you like me to handle the deprecated usages that are assigned to you?

Sure, feel free to if you're available!

zepumph commented 1 year ago

That would be so great thanks!

zepumph commented 1 year ago

Let me know if you run into trouble.

marlitas commented 1 year ago

@zepumph and @jonathanolson, the deprecated separators have been addressed in your corresponding repos.

I also deleted the deprecated separator files. If that's not the right protocol I can revert, but they are no longer being used anywhere in the codebase.

Over to @zepumph and @jonathanolson to review your repos that I touched. The changes are straightforward. I didn't run into any complications, and screens matched before/after.

zepumph commented 1 year ago

You commit has left a regression in RAP:

image

When disabled, there is no longer a dashed line. I'm not sure how you can provide that without hard coding its length (which was removed in https://github.com/phetsims/ratio-and-proportion/commit/169df9f1dc04d720d3d0dd509876c156a49edc24#diff-8ce26a7818178448309f0ea8fc0108aa25f372966ca461f2e301ef7affb23c93L63)

marlitas commented 1 year ago

Weird... I checked that and didn't see that regression... I'll look through it again.

marlitas commented 1 year ago

Fix pushed. Back to you @zepumph

zepumph commented 1 year ago

Great thanks!

samreid commented 1 year ago

I confirmed there are no more references to the deprecated separators. I updated the documentation. Closing.