Closed samreid closed 7 years ago
Here's a screenshot that depicts the unequal spacing:
When I turn on the separators it becomes more obvious why the spacing looks irregular:
The boxes are equally sized and the objects are centered in their boxes, but the items within the box are different sizes so it leads to the unequal spacing. I recall at one point @Denz1994 and I tried reordering the elements in the toolbox to fix the spacing issue but @arouinfar recommended the original ordering for pedagogical reasons (however I cannot find the issue for this). I think @Denz1994 and I also tried putting each node at the bottom of its box but that still led to irregular spacing because the light bulb is taller. I suspect this problem would disappear if the light bulb was shorter, but its current size looks pretty nice.
@pixelzoom what implementation strategy would you recommend for tweaking the distance between objects in the carousel?
You're calling this "unequal spacing", but the items are actually equally spaced. Spacing is measured from center to center of each item. And the spacing must be equal in order to ensure that each page of the carousel is the same length, so that pages can be scrolled. If you try to do what @ariel-phet has suggested, you're going to end up with either a very complicated change to Carousel, or some really ugly client code. My recommendation is to (1) mitigate the problem by increasing options.spacing
a bit so the "Light Bulb" item doesn't look as cramped, and (2) close this item as "won't fix".
The Carousel in CircuitElementToolbox is currently using spacing: 0
. When I try 5 or 10, it looks better.
Even better would be to move the Light Bulb to the bottom of the first page in the Carousel. The problem is accentuated by having it in the middle of the first page, surround by items that are similar to horizontal lines.
Light Bulb moved to the bottom of the first page, Carousel spacing: 5
. Looks great to me. I see no reason to change Carousel.
In this comment @Denz1994 and I recommended moving the light bulb to the bottom, to make the layout look nicer: https://github.com/phetsims/circuit-construction-kit-common/issues/330#issuecomment-303846084
@arouinfar replied:
(We did reduce the bulb icon size to 85%). @arouinfar and @ariel-phet how would you like to proceed?
Thanks for finding my earlier comment @samreid. I thought a battery-on-bottom carousel looked/sounded familiar. :)
After interviews, I don't think the order of the items on the first page is that important, as most students interacted with the bottom item (switch) very early on. I'll defer to @ariel-phet.
However, I would not want the high-R bulb to also be moved to the bottom of the carousel, as it would break up the grouping of the "high" components and the grab bag items.
@samreid I think shuffling the order in this case is fine, as the carousel does not really imply any particular level of importance to the items on the first page (and interviews have confirmed that).
I agree that moving the light bulb to the last item improves the spacing issue quite a bit.
I'm happy to move the light bulb to the end of page 1. What should we do for page 2? Or is it not as much of a problem?
@samreid @ariel-phet I would rather not move the high-R bulb down to the bottom of the carousel because it breaks up the grouping of the "high" components and the grab bag items.
@arouinfar I agree, but it looks like the spacing isn't as bad on page 2 (and hence maybe page 2 won't require any modification). Let's see what @ariel-phet recommends.
@samreid @arouinfar if we did move the high resistance light bulb, I think you would move the dollar bill to the 2nd position, however, in the last position it sort of suggests "grab bag items coming". However, maybe that is only meaningful to us, since we know about the grab bag items.
Can we try dollar bill in position 2 and see what that page looks like?
I moved the light bulb and the dollar, please test it here: http://www.colorado.edu/physics/phet/dev/html/circuit-construction-kit-dc/1.0.0-dev.100/circuit-construction-kit-dc_en.html
@arouinfar and @ariel-phet can you please take a look?
I really don't like the ordering of the 2nd page of the carousel. While it maintains the grouping of the "high" components (which I think is absolutely necessary), it looks very disorganized with the dollar bill in the 2nd position. The "Light Bulb" text is also really tight to the page down button (1st page has better padding).
I also think the first page would look a bit more balanced if if the switch was moved to the 2nd position. That way the battery-resistor-bulb grouping would be maintained.
@samreid can you give an estimate/strategy for improving carousel to allow some custom spacing? In terms of pixel polishing this is going to be a general problem, especially as we move forward with other versions of CCK. It seems carousel was designed with items being assumed to be the same size, and although that has been a valid assumption for sims like Function Builder, it is not a valid assumption here.
I proposed 2 strategies in https://github.com/phetsims/sun/issues/307 and provided time estimates. Reassigning to @ariel-phet to evaluate the proposed approaches and to prioritize and schedule.
Reducing priority based on @ariel-phet remark in https://github.com/phetsims/sun/issues/307#issuecomment-317567009
@arouinfar said:
I really don't like the ordering of the 2nd page of the carousel. While it maintains the grouping of the "high" components (which I think is absolutely necessary), it looks very disorganized with the dollar bill in the 2nd position. The "Light Bulb" text is also really tight to the page down button (1st page has better padding).
I also think the first page would look a bit more balanced if if the switch was moved to the 2nd position. That way the battery-resistor-bulb grouping would be maintained.
I'm hopeful that we'll be able to adjust the carousel item positions one way or another, so I restored the original ordering on page 1 and updated the ordering on page 2 to match it. You can see it in dev.101: http://www.colorado.edu/physics/phet/dev/html/circuit-construction-kit-dc/1.0.0-dev.101/circuit-construction-kit-dc_en.html
@samreid since your issue list is now shrinking, feel free to work on this issue.
Here's the before and after of applying the workaround @jonathanolson and I described in https://github.com/phetsims/sun/issues/307#issuecomment-317857763
This seems much nicer to me and hopefully won't require any further tweaking.
Dev version is here and ready for testing, @ariel-phet and @arouinfar can you please take a look?
I really like it @samreid!
Switching between the screens, it looks like the Lab carousel is just a tad taller than the Intro carousel, though.
The new spacing looks good to me @samreid
Sounds good, I'll tackle that in https://github.com/phetsims/circuit-construction-kit-dc/issues/105
From https://github.com/phetsims/circuit-construction-kit-dc/issues/82 @ariel-phet said: