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

Separate the phase diagram out of PhaseDiagramAccordionBox #313

Open
jbphet opened this issue Jul 30, 2020 · 3 comments
Open

Separate the phase diagram out of PhaseDiagramAccordionBox #313

jbphet opened this issue Jul 30, 2020 · 3 comments

Comments

@jbphet
Copy link
Contributor

jbphet commented Jul 30, 2020

PhaseDiagramAccordionBox builds the whole phase diagram inside of an accordion box, which works, but isn't a very good separation of concerns. I should pull the phase diagram into its own node which is then constructed and included in the accordion box.

@jbphet
Copy link
Contributor Author

jbphet commented Jul 31, 2020

I did some work on this and improved the situation, but in the process I realized that there is way too much coupling between the PhaseChangesScreenView and the phase diagram. This could be refactored such that the phase diagram has all the knowledge necessary to draw the diagram as long as it has a reference to the model. As it stands now, the PhaseChangesScreenView instance sets the marker position, and it has to know a lot about the diagram to do so.

I started on this refactor, but it was taking more time than I could reasonably justify. I'm going to leave it at this for now, and if there is ever a need to do more generalization of this diagram, I or someone else will need to take it from here.

@jbphet
Copy link
Contributor Author

jbphet commented Aug 7, 2020

Unassigning, since it is unlikely that I'll have any time to work on it in the foreseeable future.

@jbphet jbphet removed their assignment Aug 7, 2020
@jbphet
Copy link
Contributor Author

jbphet commented Aug 13, 2020

When this is done, care must be taken to make sure that it does not alter the phet-io API.

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

1 participant