Pixel polishing requests for the carousel

[samreid]

From https://github.com/phetsims/circuit-construction-kit-common/issues/507#issuecomment-737573013

@samreid here are the pixel polishing requests for the AC carousel. I don't know how much is shared with DC, but I am fine with any of these tweaks making their way over to DC too.

• [ ] Make carousel 10 px taller
• [ ] Reduce padding between carousel buttons and first/last item to 20 px
• [ ] Reduce large icons (light bulb, capacitor, inductor) by 10%
• [ ] Padding between icon and label is 3 px
• [ ] Equal padding between carousel items

Here's a comparison of the current (left) and proposed (right) carousels. (Note that lifelike/schematic buttons were moved down 10 px to maintain the padding between them and the carousel.)

[samreid]

One of the complications in the codebase is that the carousel spacing is recomputed when switching between lifelike/schematic in order to keep icons in the same places. Without a workaround like that (or splitting icons to schematic/lifelike and putting AlignBox around them), a fixed spacing causes all of the schematic icons would compress at the top:

Same problem if we specify a fixed spacing between elements. The spacing may work well on page 1 where there are tall elements:

But on later pages, the items collapse in the top:

I think this problem could be avoided if all items in the carousel have the same height (or at least invisible padded pox with same height).

@arouinfar can you please review and recommend how to proceed before I continue?

[samreid]

In discussion with @arouinfar and @kathy-phet, we decided to try making the icons have padded rectangles that are all the same height. We may need to make the tallest icon shorter to make this work.

[samreid]

In the commits, I separated the lifelike/schematic icons so that they can be aligned. I used AlignBox to align the lifelike/schematic icons as well as the icons across pages and screens, and to treat them as if they all have the same dimensions. I fine tuned the dimensions so that everything would fit in the worst case scenario, the lab screen of AC.

Here is the lab screen:

Here is the first screen of Circuit Construction Kit: DC:

The main free parameters for this layout are:

• The max height of icons
• The max width of icons
• The spacing between icon and text
• The spacing between adjacent items in the carousel
• yAlignment of the icons. This comment shows "bottom", see next comment for "center"

@arouinfar can you please take a look and recommend how to proceed?

[samreid]

Also, switching to iconAlignGroup.createBox( n, { yAlign: 'center' } ), (not committed) leaves less blank space at the top of the screen, but increases the space between the icons and the text (depending on icon height):

[samreid]

Also, I noticed that the schematic sizes are all disorganized (on the first page). But I'd rather get overall feedback on this strategy before fine tuning parts like that:

[samreid]

@jonathanolson and I realized that one AlignGroup is redundant, and after the commits we are using ToggleNode to manage that part. We also switched the default ToggleNode.CENTER, but could switch that to BOTTOM if preferred.

[arouinfar]

Thanks @samreid! I'm generally liking this approach but it needs some fine tuning.

I think a TOOLBOX_ICON_HEIGHT of 35 is too small, which makes icons looks very disproportionate in size. For example, the resistor feels comically larger than the capacitor.

Part of the issue is that TOOLBOX_ICON_HEIGHT might not actually be a max height (assuming width < TOOLBOX_ICON_WIDTH), or if it is, the assets have lots of unnecessary white space around them. The light bulb is the tallest component, and if I assume its height is the max height, the AC source, capacitor, and inductor all look too short. .

I think we could increase TOOLBOX_ICON_HEIGHT to 40 or 45, but we would need to tighten the space between icons a bit. However, it doesn't seem like TOOLBOX_ITEM_SPACING is actually hooked up to anything. Searching within the cck-common repo only yields a single result in CCKCConstants.js where it is defined.

[samreid]

This needs to be completed before https://github.com/phetsims/QA/issues/587, so I've updated the milestone.

[samreid]

Part of the issue is that TOOLBOX_ICON_HEIGHT might not actually be a max height (assuming width < TOOLBOX_ICON_WIDTH), or if it is, the assets have lots of unnecessary white space around them. The light bulb is the tallest component, and if I assume its height is the max height, the AC source, capacitor, and inductor all look too short.

One of the issues is that the AC source has a naturally small height, so the MAX_HEIGHT doesn't apply to it. That will be addressed in https://github.com/phetsims/circuit-construction-kit-common/issues/642. I'll need to see what's happening to the inductor and capacitor.

I think increasing the max height sounds good. I deleted TOOLBOX_ITEM_SPACING, thanks for pointing out it is unused. The new variable is CAROUSEL_ITEM_SPACING in CircuitElementToolbox.js. But note that it also determines the space before and after the first/last items. One more step we can take to make the resistor and the capacitor closer in size is to reduce the max_width--this will have the effect of shrinking the resistor a bit.

[samreid]

In the first commit, I made it so no icon will be too small. In the second commit, I adjusted the constants for size, max width, spacing and margin. It now looks like this:

@arouinfar can you please take a look and advise? I isolated the 2nd commit in case you want to see any of the adjustable parameters.

[samreid]

Eliminating the charge canvas helped fix the capacitor bounds.

[samreid]

@arouinfar and I worked on this further--ready for review. Please close if all is well.

[arouinfar]

👍🏻

[samreid]

At today's meeting @kathy-phet pointed out that it is preferable to have wider margins for the carousel. This can be seen when comparing to the published DC version, which has much wider margins:

Master:

@kathy-phet also indicated the padding over the wire seems too large.

@arouinfar and I discussed it and considered the following:

• There is extra vertical spacing between the battery and its label, which should be removed.

[samreid]

In the commits, we increased the margin by 50%, and increased the icon sizes a little bit as well. We reduced the spacing between icons (just a bit) to compensate.

Also, about the spacing above the top wire, we tried different settings for the AlignBox but "bottom" alignment was preferable. We also looked at "hacking" carousel to move everything up a little bit, but were not familiar enough with the implementation of Carousel to do so.

Let's also note that it would be possible to have different margin/icon sizes in the "DC" sim and in the "AC" sim--we didn't implement that but could try it out if desired.

@kathy-phet can you please review and advise?

[samreid]

I also opened a side issue about pruning whitespace between the battery tool icon and its text. That will help this issue somewhat.

[kathy-phet]

I like having the width with the bit of white space on either side of the icons. I think I would suggest just slightly smaller icons (mostly the height feels like it can crowd the icon above it. If a current icon is say fitting in a space that is 30 tall x 40 wide (just making these numbers up), can you adjust the space to be 25 tall x 35 wide, and then keep the overall spacing of the icon names so that there is a bit more white between, for instance, the "coin" image and the title above it "Paper Clip"? and keeping the overall width of the whole carousel the same too.

While I think a more spacious feel on the DC version would be nice, I can go either way.

I would like to know what you think about making the real world and schmatic buttons taller (more square) on the DC version. I do feel they seem pretty small, and the DC version has room.

Finally, what do you think about making the zoom buttons a bit smaller? or at least visually feel smaller by making the magnifier icon smaller. Or are these now common code buttons, and similar to what is in the voltage chart recorder?

Last, let's test whether things work OK on phones by making the touch areas as large as we can for the real/schematic buttons and seeing if the response still works on phone sizes.

[samreid]

I made it so the icon sizes can be adjusted independently, and I updated them as @kathy-phet prescribed, here is the before: after:

[samreid]

I made the lifelike/schematic buttons taller in both brands, it fits OK in AC, but we could have a different size in DC if necessary.

[samreid]

I adjusted the carousel width slightly, and updated the button dimensions in the commit. There is already code that scales each button group so its width matches the carousel, so adjusting the sizes was sort of "guess and check" and involved adjusting the spacing.

[samreid]

I increased the carousel size for the DC simulations, since it is for a younger audience it makes sense to make it more "cartoonish" and more appealing--using the sizes optimized for AC didn't seem like a good fit.

[samreid]

After increasing the pointer areas for the lifelike/schematic radio buttons and zoom buttons, I tested on iPhone and found buttons easy to press.

Regarding all of the preceding changes: We have a meeting schedule to discuss this Thursday, but let me know if you have any feedback before then.

[arouinfar]

Thanks @samreid! Let's finalize things in design meeting.

[samreid]

We agreed a scaling of 1.2 looks good for the DC carousel.

From today's meeting: @arouinfar: make the lifelike resistor a little bigger, to match the battery width. She can edit the file directly, see the preceding commit https://github.com/phetsims/circuit-construction-kit-common/commit/2d87f1f4cd609832ac7b494fee31e41121dbefb8 for guidance. @samreid: Fix the stroke of the AC voltage (create a new issue). UPDATE: if @arouinfar is creating a schematic icon for it, this seems unnecessary. @arouinfar: investigate a schematic icon for AC source (create a new issue)

[samreid]

I opened a side issue for the schematic AC source. Leaving this issue assigned to @arouinfar to revise the icon sizes as necessary.

[arouinfar]

I made some slight changes to the icon height, and they now look more balanced to me.

Here are some before (left)/after (right) comparisons:

@samreid if things look good to you, I think we can close.

[samreid]

I compared before/after in the browser and all changes seem good. Nice work @arouinfar! Closing.