create a Carousel component

[pixelzoom]

I will be needing a Carousel for Function Builder. I'm a little surprised that it doesn't already exist, since I see such a component in a couple of sims.

Questions: • Any advice on where to start? I see 3 candidates: area-builder.HCarousel, scenery-phet.KitSelectionNode, build-a-molecule.KitCollectionNode. • And are there any other variants that I missed? • Is there any difference between a carousel and a "kit"? My understanding is that "kit" is a PhET-invented name for carousel.

Assigning to @jbphet for advice, since area-builder.HCarousel appears to be the most recent.

[pixelzoom]

@jonathanolson please chime in too, since you created build-a-molecule.KitCollectionNode. Wondering why you didn't use phet.KitSelectionNode, what it lacked, etc.

[jonathanolson]

Wondering why you didn't use phet.KitSelectionNode, what it lacked, etc.

1. scenery-phet.KitSelectionNode wasn't in common code until 6 months after I made KitCollectionNode
2. KitSelectionNode would need some changes / generalization to work. Notably you can drag things out while switching kits, it doesn't have the common pattern of filled/unfilled circles at the bottom showing your position in kits (and how many kits) that is a common (and requested) UI pattern, and presumably arrow styling and position would have to change to be able to fit things horizontally. https://github.com/phetsims/build-a-molecule/issues/57 lists notes about changes to BAM's current carousel. Also notably, there is a play area for each kit in the carousel that also has to switch in BAM (which is why dragging while switching is unacceptable).

If we want to generalize that code to handle swipe-to-switch-kits (touch), customized arrow styles/locations, optional prevention of dragging while switching, and the requested circle pattern (see the bottom of http://cdn.tripwiremagazine.com/wp-content/uploads/2012/06/jquery-carousel-evolution_thumb.jpg) to show "where am I in the kits" and "how many kits are there" implicitly, I'd be happy to use it in BAM.

[pixelzoom]

@jbphet said on Skype:

In reading throught the scrollback, I saw some discussion about carousels. In answer to a couple of questions, yes, a kit is related to carousels, but we're ditching that code and the related terminology. It was left over from a couple of Java sims. HCarousel is the most recent one, and we had decided to generalize it as soon as someone needed it outside of Area Builder. So yes, I think that would be a good starting point.

[pixelzoom]

@jbphet JO said he didn't use yours because his preceded yours. So I'll ask you the same question, why didn't you use his? I'm trying to understand how they differ, what features you didn't have, etc. so I can generalize.

[jbphet]

I'm not sure I was even aware of the existence of a working carousel in Build a Molecule when I implemented HCarousel for Area Builder. We discussed HCarousel in several design meetings, and I can see it mentioned in the 2014 notes, and we said that it should be moved to common code at some point, so I tried to keep that in mind as I did the implementation. I don't think using the one from BAM ever came up.

Looking at the code now, I see that KitPanel has some unique behaviors, such as being able to refill the kits, so I'm not sure it's what we would want for the generalized version. Maybe that's why we didn't go there.

Note that we also talked about making it simply "Carousel" and having an orientation option. You could do this for now and leave out the vertical version until some lucky winner needs it.

At any rate, I trust @pixelzoom's judgement - use whichever looks like a better starting point for generalization and run with it.

[pixelzoom]

@jbphet wrote:

Note that we also talked about making it simply "Carousel" and having an orientation option. You could do this for now and leave out the vertical version until some lucky winner needs it.

Function Builder has vertical carousels, so I'm the lucky winner.

I guess I could also just write my own VCarousel and leave it to the next person to generalize.

[samreid]

I guess I could also just write my own VCarousel and leave it to the next person to generalize.

For vertical sliders, it has worked surprisingly well to use a rotated HSlider. Not sure if that will be possible/useful for carousels.

[samreid]

@AshrafSharf will also need a carousel for the Micro screen of Sugar and Salt Solutions. @pixelzoom can you report on your priority for this issue? Is it pushed back because of Hooke's Law?

[pixelzoom]

I'm planning to work on Function Builder when I have down time from Hooke's Law. No idea yet how much or when that will be. As for the carousel, I will probably start by creating what I need for Function Builder, and generalize later (probably much later). If there are requirements for SASS that should be considered, someone from that team should formalize those requirements herein.

[pixelzoom]

@amanda-phet you've been identified as the lead on "Sugar and Salt Solutions" (SASS). Please describe the requirements for the carousel in SASS.

[pixelzoom]

@amanda-phet Btw... This is not urgent. Address at your leisure.

[amanda-phet]

The carousel for SASS is a version of the one used in Balancing Act. It need to fit the scroll buttons in to a tight space, which is why they can't run the full vertical height of the rounded rectangle. One change from Balancing Act, though, is the light gray, inactive direction if you are at the beginning/end of the carousel.

The carousel in Function Builder is different, but should be our common carousel moving forward (and also replace the one used in Area Builder).

[pixelzoom]

Graphing Quadratics has been moved ahead of Function Builder, so I will not be working on this in the foreseeable future. Unassigning.

[pixelzoom]

See duplicate issue https://github.com/phetsims/sun/issues/56 for additional discussion and requirements.

[pixelzoom]

@ariel-phet There are multiple designs and implementations of carousel floating around. So step one is to review the existing variants, and come up with a unified design. Assigning this to you to decide which designer should lead the design effort.

As for Function Builder... Based on what I know about the schedule for Function Builder, we can't let this delay implementation. If the unified design is ready for me to implement for Function Builder (which I'm resuming work on now), then I'll implemented it. Otherwise I'm going to implement yet-another variant based on the requirements in the Function Builder design document.

Does that sound like a reasonable plan?

[ariel-phet]

@pixelzoom - see above, the design in function builder should be the common carousel, and should replace the one in area builder. We may need two types of carousels, one that is like the one in function builder (which will also be in Area Builder, and build a molecule). This is sort of the large carousel that tends to go at the bottom of the screen. We might need a second implementation at some point that is for sims like Balancing act and SASS.

Would it be wrong to have one that is carousel, and another that is something like ControlPanelCarousel? Do these two really need to be unified?

[pixelzoom]

@ariel-phet wrote in https://github.com/phetsims/sun/issues/166#issuecomment-138795016:

the design in function builder should be the common carousel

If that's true, then I should be able to proceed without looking at other variants (none of which matches the design in Function Builder), and leave it to others to replace their implementations with what I implement for Function Builder. I'll be happy to proceed under that assumption, if that's what you want.

But I'm skeptical (a) that anyone has reviewed the multiple variants of carousels/kits that exist, (b) that anyone has verified that Function Builder's carousel is general enough to work in the current (and future) sims that have carousels/kits.

Below is (to my knowledge) the first review of the various carousels and kits that PhET has. They are all different in significant ways. And there are problems that are immediately apparent if we try to substitute Function Builder's carousel for all of these cases.

(1) Area Builder (area-builder.Carousel): horizontal orientation, arrow buttons are the same height as the carousel, arrow button disappears when can't scroll further, scrolling animates when arrow button is pressed. Closest to Function Builder of any of the variants.

(2) Build A Molecule (build-a-molecule.KitCollectionNode): horizontal orientation, both arrow buttons in upper-right, arrow button disappears when can't scroll further, no animation when arrow button is press (next kit just suddenly appears). Very different than Function Builder.

(3) Balancing Act (scenery-phet.KitSelectionNode): horizontal orientation, arrow buttons at left and right, arrow buttons are not the full height of the carousel, arrow button disappears when can't scroll further, scrolling animates when arrow button is pressed. Different form factor than Function Builder, particularly buttons.

(4) Sugar and Salt Solutions (not implemented yet, but see https://github.com/phetsims/sun/issues/166#issuecomment-94328663): appears to be a variant of the one used in Balancing Act, but with arrow buttons in a different location.

(5) Function Builder (as proposed in design doc): horizontal and vertical orientations, arrow buttons are the same width/height as the carousel, arrow buttons gray out (do not disappear) when can't scroll further, scrolling animates when arrow button is press, optional (?) separator line between items in the carousel.

So... @ariel-phet How do you want to proceed with this issue?

[ariel-phet]

@pixelzoom, as I mentioned above, I think there may need to be more than one variant of the carousel to fit different purposes. I think what has been designed for function builder fits our current design paradigm and will work well for area builder. I also think that design could work for build a molecule, with some design tweaks to BAM. I am not claiming that will work for what is implemented in balancing act/SASS, and I think that may need to be thought of as a different common component. I do not think in this particular case it is wise to try to design a common carousel for all these different use cases. The one for function builder will fill many.

If you think it makes sense to have a single, completely flexible implementation, with multiple options that allow for labels, button colors, different button placement, and such, we can discuss further, but that seems unwise at this juncture.

I am imagine we can take what you do and build a different common component that fits use cases like Balancing Act and SASS, and try to work with those as we move forward.

[ariel-phet]

This will also be the implementation planned for CCK basics.

It does seem one option people have wanted is "dots" on the bottom of the carousel (the sort of apple standard of swiping between multiple screens to designate what screen you are on within an app), so I would include that option with the implementation. (mentioned in a comment above by @jonathanolson)

[jonathanolson]

Also note that in Build a Molecule (2), switching to other "kits" also changes what is seen in the play area (there is essentially one play area per kit).

[pixelzoom]

Somewhat related to https://github.com/phetsims/scenery-phet/issues/186 (drag handler for items in a toolbox/carousel/bucket).

[pixelzoom]

@ariel-phet About the "dots" feature, requested in https://github.com/phetsims/sun/issues/166#issuecomment-139022059...

(1) Where do you want the dots positioned? Inside or outside the carousel? For vertical carousels, on the left or right? For horizontal carousels, above or below?

(2) If dots are inside the carousel... The "separators" feature (see screenshot below) conflicts with the "dots" feature. Please advise on how you want this to looks when "separator" and "dots" are requested.

[ariel-phet]

@pixelzoom I think @jonathanolson had a good shot of the convention in a comment above http://cdn.tripwiremagazine.com/wp-content/uploads/2012/06/jquery-carousel-evolution_thumb.jpg

For horizontal, dots should definitely be outside and below

For vertical, I am not sure if we want to have the dots option or not. But if we were going to have such an option, for vertical we would probably need the freedom to place them on the left or the right depending on sim layout. Although we will likely have other sims with vertical carousels, I envision them being more rare.

@amanda-phet should chime in with her thoughts.

[pixelzoom]

For vertical, I am not sure if we want to have the dots option or not.

In the specific case of Function Builder, we have both horizontal and vertical carousels. @amanda-phet are you wanting the "dots" feature in Function Builder, and do you want it for both horizontal and vertical carousels?

I can imagine why we might now want dots for the vertical carousels in Function Builder, since they are pseudo tables. But in general, I don't know why we wouldn't want this as an option for both orientations.

[pixelzoom]

Note to self... The "dots" feature will make relative layout of carousels difficult. Consider: • For horizontal carousels, put y=0 at the vertical center of the carousel • For vertical carousels, put x=0 at the horizontal center of the carousel

[amanda-phet]

We did not plan on using dots in Function Builder. I'm open to it, but it was not in the design.

However, in a general carousel design meeting, we decided that it should be an option in the common carousel. They would be below and outside of a horizontal carousel, and could go on the right or left of a vertical carousel, depending on the placement in a sim.

[pixelzoom]

FYI... What we've been calling the "dots" features is formally known as a "page control" in the iOS Human Interface Guidelines. See https://developer.apple.com/library/ios/documentation/UserExperience/Conceptual/MobileHIG/Controls.html#//apple_ref/doc/uid/TP40006556-CH15-SW6

[pixelzoom]

Highlights from a Skype discussion about carousel requirements and implementation...

Regarding whether to use clipping to determine which items are visible:

[9/14/15, 1:00:17 PM] Chris Malley: Question for JO and JB... I started by patterning sun.Carousel on area-builder.HCarousel, because it has the closest visual appearance. I see that HCarousel adds all nodes to the scenegraph and uses a clipping window to display a subset of those nodes in the carousel. Any performance concerns in general with this approach, for example with large numbers of complicated nodes? [9/14/15, 1:00:49 PM] Chris Malley: It certainly makes animation (scrolling) easier. [9/14/15, 1:05:37 PM] John Blanco: I don't know if there are performance issues. It worked well for Area Builder, and I've used clipping in a number of other sims with no obvious performance issues. WebGLNode doesn't support clipping yet, and I've had to work around that in Neuron, but I don't think that should be an issue for what you're doing. JO will have to be the authority on the performance costs for Scenery. [9/14/15, 1:19:44 PM] Jonathan Olson: Clipping sounds like the best way to handle the carousel right now.

Regarding whether to display empty cells in the carousel:

[9/14/15, 1:09:39 PM] Chris Malley: Another "requirements" opinion needed... If the carousel can show 4 items at a time, and we have 9 items, is the 9th item shown alone, or with the 3 before it? Ie, are there every empty cells in the carousel? [9/14/15, 1:18:03 PM] Sam Reid: carousel API should support “blanks” [9/14/15, 1:18:25 PM] John Blanco: Visually, I would vote for the "no empty cells" option, so perhaps it should be configurable. [9/14/15, 1:18:46 PM] Chris Malley: area-builder shows empty cells, so I'm surprised to hear that. [9/14/15, 1:19:07 PM] John Blanco: Does it? Let me look... [9/14/15, 1:19:28 PM] Chris Malley: you seem to always have a multiple of 4 items, so you don't see blanks in the sim. [9/14/15, 1:20:15 PM] John Blanco: Right, so it didn't really come up for AB. [9/14/15, 1:25:00 PM] John Blanco: The blank spaces look a little odd to me, like maybe it was a bug, but it's not horribly bothersome. I'd say go with whatever is easier for now. [9/14/15, 1:33:03 PM] Chris Malley: If you don't leave blank spaces, then you'll see different sets of items depending on the direction of scrolling. Eg, if you have 4 spaces and 13 items, then in the "next" direction you'll see items 1-4, 5-8, 9-12, 10-13. Scrolling in the "previous" direction, you'll see 10-13, 8-11, 4-7, 1-4. The end sets will be the same, but all of the middle sets will be different. [9/14/15, 1:33:40 PM] Chris Malley: That doesn't work well if the carousel is supposed to support "kits", where each visible collection is related. [9/14/15, 1:34:08 PM] John Blanco: True. Looks like blank spaces are probably a better option.

Based on the above, I've gone with the following decisions:

(1) The implementation of carousel will rely on a clipping area. This may have performance implications for carousels with large numbers of items.

(2) The items in carousels will be divided in "pages", each page having the same number of cells. If the number of items is not an integer multiple of the number of cells, then the last page will contain empty cells.

[pixelzoom]

Grrrr... Accidentally migrated Carousel and its demo to scenery-phet, when it belongs in sun. I'll move it.

[pixelzoom]

All requests features are implemented, and a demonstration lives in the "Components" screen of the sun demo application (see screenshot below).

There are 3 related source files:

• Carousel.js is the main carousel component
• CarouselButton.js is the button uses by Carousel
• PageControl.js is the iOS-style page control (aka "dots") used by Carousel

As requested, there are a boatload of options to Carousel, see DEFAULT_OPTIONS in https://github.com/phetsims/sun/blob/master/js/Carousel.js.

[pixelzoom]

@ariel-phet I'm transitioning back to Function Builder, where I will be immediately making heavy use of Carousel. Can you please assign someone to review this work? Recommended to first look for feature gaps, then review the implementation.

[ariel-phet]

@pixelzoom sounds good.

I think @jbphet is the natural reviewer considering his use of a carousel in area builder and in balancing act

@jbphet please review as a high priority.

[pixelzoom]

@jbphet Perhaps as part of the review you could try replacing area-builder.HCarousel with sun.Carousel, to address https://github.com/phetsims/area-builder/issues/81.

[pixelzoom]

Back to me, I just discovered a missing feature. In Function Builder, we need the ability to replace an item in the "outputs" carousel.

I'm also wondering what other features might be needed in general related to mutability of the items array, or of the items in the array - there has been nothing specified wrt to that. The items are currently specified when the carousel is constructed, and both the items and the array that contains them are considered to be immutable. Let's discuss requirements at developer meeting.

[pixelzoom]

Ah, sorry, no feature gap for Function Builder. Each cell in the carousels (inputs, outputs and functions) needs to contain a node that manages what's in the cell, so that cards/functions can be dragged out (much like a toolbox).

But still worth discussing the mutability of items and items array at developer meeting.

[pixelzoom]

Also for dev-meeting discussion... Enabling the page control feature makes it difficult to visually center things on the "body" of the carousel. Let's discuss how important this is, and how to address it.

For example, here's my attempt to horizontally center an EraserButton on a vertical carousel in function-builder. The button is (understandably) to the left of the desired location, because the code looks like:

// Eraser button, centered below the output carousel
var eraserButton = new EraserButton( {
  ...
  centerX: outputsCarousel.centerX,
  top: outputsCarousel.bottom + 10
});

[pixelzoom]

A couple of options for addressing the layout issue caused by enabling the 'page control' feature in Carousel:

(1) Set Carousel's origin (0,0) to be the center of the carousel's scrolling area. Then do layout using x and y properties, like this:

// Eraser button, centered below the output carousel
var eraserButton = new EraserButton( {
  ...
  centerX: outputsCarousel.x,
  top: outputsCarousel.bottom + 10
});

(2) Remove the page control option from Carousel (it's already factored out in PageControl.js), and have clients handle creating it. Example:

var orientation = 'vertical';
var carousel = new Carousel( items, {
   orientation: orientation,
   ...
}  );
var pageControl = new PageControl( carousel.pageNumberProperty, {
   orientation: orientation,
   centerX: carousel.centerX,
   top: carousel.bottom + 10
} );

[pixelzoom]

Consensus at 9/17/15 dev meeting: • immutable items OK for now, deal with it later if need arrives • remove page control option for Carousel.

[pixelzoom]

PageControl features have been removed from Carousel, interactivity has been added to PageControl (see #199), and a demo for PageControl has been added to sun.

Ready for review by @jbphet.

[pixelzoom]

@jbphet Reminder that this is assigned to you for review, with high priority.

[jbphet]

It's on my list for today.

[pixelzoom]

There is one TODO item in Carousel.js that deserves explanation:

//TODO should we put a wrapper node around each item, in case the item appears elsewhere in the scenegraph?

Carousel currently sets layout properties (centerX, centerY) of the items that it contains. If those items are used elsewhere (taking advantage of the DAG features of scenery), that will cause problems. My inclination is to leave it as is, and address it if someone has a need to reuse the items. The alternative is to wrap each item in a Node, and then position the wrapper Node.

[samreid]

I think it is a fair assumption for carousel to be able to position the items it is given. If an application wants to DAG, then it is the application's responsibility to provide the carousel with wrapped nodes.

[pixelzoom]

Agreed. I've replaced the TODO item with this note in the JSdoc:

* Note that Carousel performs layout directly on the items (Nodes) that it is provided.
* If those Nodes appear in multiple places in the scenegraph, then it's the client's
* responsibility to provide the Carousel with wrapped Nodes.

[jbphet]

Code looks great - clean, well documented, and makes nice use of underscore/lodash. A few initial comments:

• The documentation in Carousel.js says that the fill option can validly be set to null, but doing so causes an assertion because it is being passed through to the button.
• The arrow stroke option isn't available when constructing Carousel.js. Balancing Act using a white stroke for the arrows, other sims may want to do this as well.
• Do we want to add some touch and mouse area expansions to the PageControl? I just tested on an iPad, and the page control does work for switching screens, and it might be tricky to touch these little circles if the touch area isn't expanded a bit.
• Can the unused index value be deleted from the forEach loop currently on line 118 of Carousel.js?

I tried modifying a number of different options and varied the size of the nodes going into the carousel, and everything worked fine. Here's a screen shot as a record of some of the things tested:

I haven't integrated this component into Area Builder yet, will do so tomorrow or Friday.

[pixelzoom]

The documentation in Carousel.js says that the fill option can validly be set to null, but doing so causes an assertion because it is being passed through to the button.

Sounds like a deficiency in buttons. Why can't a button have a null fill?

The arrow stroke option isn't available when constructing Carousel.js. Balancing Act using a white stroke for the arrows, other sims may want to do this as well.

I'll add this option.

Do we want to add some touch and mouse area expansions to the PageControl? I just tested on an iPad, and the page control does work for switching screens, and it might be tricky to touch these little circles if the touch area isn't expanded a bit.

The interactivity features of PageControl are still being discussed, see https://github.com/phetsims/sun/issues/199. I've copied this comment to that issue.

Can the unused index value be deleted from the forEach loop currently on line 118 of Carousel.js?

Yes, I'll take care of this. The index parameter is vestigial from the port of area-builder.HCarousel, see line 110 of that type.

[pixelzoom]

Issues that @jbphet brought up are addressed in the commits immediately above.

Back to @jbphet to integrate into Area Builder.

[jbphet]

This has been integrated into Area Builder, and works perfectly. I think that's it for this - thanks @pixelzoom . Closing.

[pixelzoom]

Thanks for the review.