-
Notifications
You must be signed in to change notification settings - Fork 2
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
Constants and colors in Util.js #21
Comments
UnitCircleView would also benefit from renaming to TrigTourColorScheme because Util would not have to be renamed to UtilTrig. It is a little confusing when the var name does not match the module name. To me, UtilTrig means a file that holds utility trig functions. TrigTourColors would more accurately describe the file
|
@amanda-phet, are you familiar with the color-blind green that is being used to represent sin? Was this color tested for visibility in color-blind tests? If it was tested, I think this color should be added to the PhET color scheme. If not, some documentation needs to be changed. |
We chose the green while doing color-blind tests, so it does behave well with the red. I'm not sure if the red is exactly color-blind-red. Did you verify the hex code or RGB components? If it is, then feel free to add the green as a good green to go with it! |
I verified that the red in trig-tour has the exact RGB components of RED_COLORBLIND in PhetColorSCheme. I added the green-colorblind to PhetColorScheme for future use since it pairs well with red-colorblind. That completes this issue, closing. |
Related to code review #14 and somewhat related to #20.
It looks like TrigTourScreenView.js uses the default layout bounds, so can constant LAYOUT_BOUNDS be removed from Util.js? If the constant can be removed, I would recommend renaming Util.js to something a little more descriptive like TrigTourColors.js.
For the colors, TAN_COLOR should be pulled from RED_COLORBLIND in PhetColorScheme.js and renamed to RED_COLORBLIND.
SIN_COLOR is documented as color-blind green. I am not familiar with color blind green, did this color test well in colorblind tests? If yes, then I think hex color '#008700' should be moved to PhetColorScheme and documented there.
The text was updated successfully, but these errors were encountered: