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

Reimplement ChallengeComboBoxItem #484

Closed
pixelzoom opened this issue Jun 24, 2022 · 4 comments
Closed

Reimplement ChallengeComboBoxItem #484

pixelzoom opened this issue Jun 24, 2022 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

In phetsims/sun#768, ComboBoxItem is being converted from class to type.

From phetsims/sun#768 (comment):

Hmmm... This makes converting ComboBoxItem to a type a bit more complicated. In ratio-and-proportion:

class ChallengeComboBoxItem extends ComboBoxItem<number> 

... and ChallengeComboBoxItem does some work that a type cannot.

ChallengeComboBoxItem is a bad design. The responsibility for setting the background color of the sim should not be in the item. The item should select a value, and setting the background color based on the value should be handled elsewhere. @zepumph may I change this?

What I said about ChallengeComboBoxItem is true in general. Items should not have responsibilities like this.

@pixelzoom pixelzoom self-assigned this Jun 24, 2022
@pixelzoom
Copy link
Contributor Author

In phetsims/sun#768 (comment), @zepumph said:

Yes that makes sense. Go ahead and make that change. Thanks!

@pixelzoom
Copy link
Contributor Author

Since this is a significant change, I published 1.2.0-dev.24 as a baseline before proceeding.

@pixelzoom
Copy link
Contributor Author

Done in the above commit.

I noticed that there was a lot of duplication of the ratio constants 1/2, 1/3, 3/4 in ChallengeRatioComboBoxNode. There was an existing Map that mapped ratio to capitalized and lowercase strings. So I extended that Map to include other info about challenges that is needed by ChallengeRatioComboBoxNode. That eliminated the duplication of ratio constants, and facilitates creating the ComboBoxItems by iterating over the map.

@zepumph please review.

@zepumph
Copy link
Member

zepumph commented Jun 27, 2022

Looks amazing (obviously). Thanks for doing that refactor. I really appreciate. The code is much better.

@zepumph zepumph closed this as completed Jun 27, 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

2 participants