-
Notifications
You must be signed in to change notification settings - Fork 6
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
Is too much of OhmsLawScreenView.js empirically determined? #55
Comments
@jessegreenberg would be a good person. It should be noted that this sim was pretty early in HTML5 land, and was originally done by MLL, so it could likely benefit from multiple improvements. |
I'm not the best guy to determine exactly where these should be, but I will make issues with my best guesses. Should I ask @jessegreenberg to clarify all of the potential shortcomings? |
I would say the code review checklist should be a pretty good start....if the sim conforms well to the checklist and our style guidelines that is pretty solid. |
Alright, that sounds good. @jbphet did a substantial amount of work on it about 18 months ago. That combined with @veillette's work in May has made it pretty good already. |
@zepumph, happy to look into this with you, but "empirically determined" layout values are actually common in most ScreenView types. We don't usually model the view layout for UI components. For instance A common layout strategy for PhET is to place things in the view relative to other things, like the |
Sounds great thanks! |
from code review: #51 |
There are 5 "empirically determined" values in the screen view effecting 3/5 view components in the sim. Is this too much? Should sims have a more robust layout scheme, even when the layout is static? Should effort be taken to minimize this? I'm not sure how to, but if someone recognizes it as a problem, I would be happy to pair for some advice on tackling the problem.
Assigning to @ariel-phet. Could you please determine an experienced developer that is not overbooked currently that may be able to take 5-30 minutes to help me with this issue. Thanks!
The text was updated successfully, but these errors were encountered: