phetsims / sun

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

Address uses of `any` for TButtonAppearanceStrategy and TContentAppearanceStrategy #754

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

There are several uses of any in sun, all related to TButtonAppearanceStrategy and TContentAppearanceStrategy (types created by @zepumph). These should be replaced with correct types.

Occurrences of any in:

... and possibly elsewhere?

Assigning to @zepumph and @jbphet (the original author).

jbphet commented 1 year ago

After making the commits shown above, I closely compared the buttons to those found in published versions of the sims, and they look a little different. I'm not sure that this is the result of my changes, because I didn't check prior to making the changes, but I could back them out and find out for sure if necessary. But the differences are fairly subtle, and might not really matter, so I'm going to ask around a bit before spending much time on this. Here is a before-and-after comparison for some buttons Molecules and Light:

MAL 1.5.3:

image

MAL on master as of today:

image

These are from Chrome version 105.0.5195.127 .

The stroke on the round buttons looks a bit more pronounced, and on the rectangular button it looks a little less pronounced.

jbphet commented 1 year ago

I reviewed the visual changes with @arouinfar and @pixelzoom (in separate discussions), and we all agreed that they are insignificant enough that we don't need to investigate why the differences exist. Assigning to @pixelzoom to review.

pixelzoom commented 1 year ago

I’m still seeing “appearance strategy” problems with zoom buttons. They are missing stroke. For example, in Natural Selection:

screenshot_1898

They should look like this:

screenshot_1899
arouinfar commented 1 year ago

The examples I reviewed with @jbphet didn't include a case where two buttons are in direct contact, like the zoom buttons The lack of a stroke separating the zoom buttons seems problematic to me.

jbphet commented 1 year ago

Here is another instance from the bamboo repo demo:

image

jbphet commented 1 year ago

The problem with missing strokes for Zoom buttons using the flat appearance strategy was because in the course of porting this code to TypeScript, I'd changed to code to only create a default stroke if the provided stroke was undefined. The previous code created a default stroke when any falsey value, including null, was provided as the stroke, and some of the base classes set this value explicitly. I have changed the code to be more like the previous code, but I acknowledge that this is a bit inconsistent with other nodes, where a null value for the stroke means "don't use a stroke on this node". As with https://github.com/phetsims/sun/issues/772, I'm making a decision to go with backward compatibility over perfect consistency.

@pixelzoom - Please continue the review and let me know if you see anything in the code or the sims that look problematic. When you're done, assign this back to me and I'll request that QA go through the sims and look for any other cases of buttons that look like they are missing strokes.

pixelzoom commented 1 year ago

I took a quick look through the code and commits, and I don't see anything that looks problematic. The example in natural-selection and bamboo demo also look OK now.

Back to @jbphet in case there's anything else to do here.

jbphet commented 1 year ago

Thanks for the review, I think that should do it. Closing.