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

Pixel polishing #240

Closed
11 tasks done
arouinfar opened this issue Mar 11, 2020 · 14 comments
Closed
11 tasks done

Pixel polishing #240

arouinfar opened this issue Mar 11, 2020 · 14 comments

Comments

@arouinfar
Copy link
Contributor

arouinfar commented Mar 11, 2020

@jessegreenberg here are the recommended pixel polishes. Where applicable, the changes would also affect ESPB.

  • Use consistent corder radii for all panels/accordion boxes/legends, let's try 8 px
  • Use 14 pt font for checkboxes
  • Size of checkboxes is generally too large and should match the text height
  • Left-align checkboxes with "Friction" in the main control panel.
  • Increase padding between checkbox and corresponding icon, so the appearance is more like the published version of ESPB. (Not sure if this possible with alignBox implementation, feel free to push back.)

  • Add thin gray stroke (#ababab) to main control panel, toolbox, Energy AccordionBox (Graphs screen), grid panel. (All gray panels should have this stroke.)
  • Change fill/stroke color of Bar Graph AccordionBox to match pie chart legend.
  • Change fill/stroke of Grid/Ref Height panel to match main control panel.
  • Remove dilation from PlayPauseButton
  • Reduce size of PlayPauseButton and StepForwardButton by 10%. Relative sizes look good in this screenshot, just a bit too large overall.
    image
  • Change color of ProbeNode to match Measure tool body color.
@jessegreenberg
Copy link
Contributor

jessegreenberg commented Mar 26, 2020

When I increased the font size of checkboxes I noticed that the grid/reference height panel becomes wide enough to occlude grid labels. @arouinfar what should we do?

image

EDIT: I thought about shifting the labels to the other side of the vertical line but then they would be occluded by the energy legend.
image

@arouinfar
Copy link
Contributor Author

arouinfar commented Apr 2, 2020

When I increased the font size of checkboxes I noticed that the grid/reference height panel becomes wide enough to occlude grid labels. @arouinfar what should we do?

Can we move the 0 m marker to the lower right corner of the first grid square (just above ground level)? It looks like all the other height measurements are in the lower right corner, just above the line they are labelling.

Screen Shot 2020-04-01 at 10 18 17 PM

@jessegreenberg
Copy link
Contributor

Can we move the 0 m marker to the lower right corner of the first grid square

Good idea! Done.

@jessegreenberg
Copy link
Contributor

This list of changes is done @arouinfar. Reassigning to you to review.

@arouinfar
Copy link
Contributor Author

arouinfar commented Apr 23, 2020

Thanks @jessegreenberg! The sim is definitely looking cleaner, but I found a few more pixels to polish.

  • Increase font size of Height = 0 label on the reference line to 14 pt.
  • The graphic return skater buttons still have the old skater artwork on them. Can you use Skater 1 instead?
  • The background of the Energy bar graph AccordionBox and the pie chart legend should be white. (Please leave the Energy AccordionBox on the Graphs screen gray since the graph itself has a white background already.)
  • Reduce corner radii to 5 pt.
  • Change color of energy type strings (Potential, Kinetic, etc.) to black in the Pie Chart legend and in the Bar Graph labels.

EDIT: Energy Graph AccordionBox polishes moved to #262.

@jessegreenberg
Copy link
Contributor

Change color of energy type strings (Potential, Kinetic, etc.) to black in the Pie Chart legend and in the Bar Graph labels.

Hmm, it turns out that griddle forces these labels to match the bar color so Ill need to make changes there to do this.

@arouinfar
Copy link
Contributor Author

arouinfar commented Apr 30, 2020

Hmm, it turns out that griddle forces these labels to match the bar color so Ill need to make changes there to do this.

I would be fine making the labels black across all energy graphs. I suspect that the colors do not have sufficient contrast against light gray or white backgrounds to pass at the WCAG AAA level. https://webaim.org/resources/contrastchecker/

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 30, 2020

Oh I see - in that case other sims that use BarChartNode are masses-and-springs and pendulum-lab. I would change default behavior in griddle to have black label text, but you could optionally pass in a different color. @arouinfar can you please confirm that you are OK changing labels to black in all three sims? EDIT: Or if you would like to leave colors as they are for one sim.

@jessegreenberg
Copy link
Contributor

Questions in #240 (comment) will be moved to phetsims/griddle#49.

The labels are now black. @arouinfar re-assigning to you to review.

@arouinfar
Copy link
Contributor Author

@jessegreenberg the updated labelColor looks good in master. I've also commented in phetsims/griddle#49.

@arouinfar arouinfar assigned jessegreenberg and unassigned arouinfar May 1, 2020
@arouinfar
Copy link
Contributor Author

I just realized that all items in #240 (comment) were checked off, so I reviewed those in master.
Everything's looking great @jessegreenberg!

I don't think there's any remaining work for this issue, so I'll go ahead and close.

@arouinfar
Copy link
Contributor Author

arouinfar commented May 1, 2020

So sorry to re-open so quickly, but it seems appropriate to group this last set of requests in with this issue.

The font sizes in the Energy Sensor on the Measure screen all seem a bit too small compared to other text in the sim.
image

  • "Energy" title: 16 pt
  • All other labels/values (including height/speed): 14 pt

I realize this means the tool body will end up a bit bigger, but there's plenty of room for it on this screen.

@jessegreenberg
Copy link
Contributor

OK, text sizes were increased. @arouinfar can you review again?

@arouinfar
Copy link
Contributor Author

Thanks @jessegreenberg! I think the data will be much more readable when the sim is being projected in a classroom.
image

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