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

Address uses of any for TButtonAppearanceStrategy and TContentAppearanceStrategy #754

Closed
6 tasks done
pixelzoom opened this issue Apr 11, 2022 · 8 comments
Closed
6 tasks done

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 11, 2022

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:

  • TButtonAppearanceStrategy
  • TContentAppearanceStrategy
  • ButtonNode.FlatAppearanceStrategy
  • RectangularButton.ThreeDAppearanceStrategy
  • RectangularButton.ContentAppearanceStrategy
  • RoundButton.ThreeDAppearanceStrategy

... and possibly elsewhere?

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

jbphet added a commit to phetsims/circuit-construction-kit-black-box-study that referenced this issue Jul 21, 2022
@jbphet
Copy link
Contributor

jbphet commented Sep 28, 2022

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
Copy link
Contributor

jbphet commented Oct 3, 2022

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.

@jbphet jbphet assigned pixelzoom and unassigned jbphet and zepumph Oct 3, 2022
@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

jbphet commented Oct 5, 2022

Here is another instance from the bamboo repo demo:

image

@jbphet
Copy link
Contributor

jbphet commented Oct 5, 2022

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 #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.

@jbphet jbphet assigned pixelzoom and unassigned jbphet Oct 5, 2022
@pixelzoom
Copy link
Contributor Author

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.

@pixelzoom pixelzoom assigned jbphet and unassigned pixelzoom Oct 16, 2022
@jbphet
Copy link
Contributor

jbphet commented Oct 17, 2022

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

@jbphet jbphet closed this as completed Oct 17, 2022
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