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

Evaluate "centralized code with options" vs "extendable base classes" for this sim #153

Closed
samreid opened this issue Apr 22, 2023 · 7 comments

Comments

@samreid
Copy link
Member

samreid commented Apr 22, 2023

While implementing #149, it seems the sim is more based on options and having parent types implement things (like the accordion box), but this is making it difficult to adjust and customize for the Variability screen. For instance, we need to add different checkboxes, an info button and move the left edge to the right. Should all of this be through options, or should the VariabilityScreenView create its own CAVAccordionBox that it customized appropriately? I want to discuss this with @marlitas and/or @matthew-blackman before working on it, because there are tradeoffs either way and it would be good to discuss before spending a lot of time on it.

@samreid samreid added the dev:help-wanted Extra attention is needed label Apr 22, 2023
@samreid
Copy link
Member Author

samreid commented Apr 25, 2023

This issue has some overlap with #152

@samreid
Copy link
Member Author

samreid commented Apr 27, 2023

I feel the last place where this issue applies is in the readouts on the left of the accordion box for screens 2-3. @marlitas was working on that, I believe.

@marlitas
Copy link
Contributor

marlitas commented May 2, 2023

Completed the readouts in accordion boxes. Over to @samreid for review, and perhaps closing this off together.

@samreid
Copy link
Member Author

samreid commented May 6, 2023

Commits above look good. The proposed changes in #170 also match this pattern. I feel this issue can be closed.

@samreid samreid closed this as completed May 6, 2023
@phet-dev phet-dev reopened this May 7, 2023
@phet-dev
Copy link
Contributor

phet-dev commented May 7, 2023

Reopening because there is a TODO marked for this issue.

@samreid
Copy link
Member Author

samreid commented May 10, 2023

All TODOs addressed. Ready for review. In my opinion, I feel there are still some places that could use refactoring like this, but we can address them if/when discovered. @marlitas can you please review recent commits and close if all is ok? Or comment if you know of other places that should be refactored?

@marlitas
Copy link
Contributor

Reviewed with @samreid. Any other work regarding this topic will be done on an as-needed basis. We are ready to close.

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