Closed samreid closed 1 year ago
At today's design meeting, we agreed we need to make the distinction between "all the batteries are currently gone and there should be a placeholder blank spot for them" vs "the customized sim doesn't have any batteries so there shouldn't be a spot for them".
@kathy-phet said this may be beyond the scope of our January 2022 sprint.
The pagination is very explicit at the moment, with wires at the top of each page.
const circuitElementToolNodes = [
circuitElementToolFactory.createWireToolNode(),
circuitElementToolFactory.createRightBatteryToolNode(),
circuitElementToolFactory.createACVoltageToolNode(),
circuitElementToolFactory.createLightBulbToolNode(),
circuitElementToolFactory.createResistorToolNode(),
circuitElementToolFactory.createSwitchToolNode(),
circuitElementToolFactory.createWireToolNode(),
circuitElementToolFactory.createFuseToolNode(),
circuitElementToolFactory.createDollarBillToolNode(),
circuitElementToolFactory.createPaperClipToolNode(),
circuitElementToolFactory.createCoinToolNode(),
circuitElementToolFactory.createEraserToolNode(),
circuitElementToolFactory.createWireToolNode(),
circuitElementToolFactory.createPencilToolNode(),
circuitElementToolFactory.createHandToolNode(),
circuitElementToolFactory.createDogToolNode()
];
It's unclear what the desired behavior should be during customization. Should the client be able to set up different pagination during startup? Or dynamically?
@kathy-phet said this may be beyond the scope of our January 2022 sprint.
Is the current behavior acceptable for now?
@samreid @arouinfar - I would say that this is not a priority for the Jan 2022 sprint, and can defer for a later time.
@arouinfar should this be part of the Full initial PhET-iO release: DC Milestone? How should we proceed?
This is definitely worth investigating before the full publication milestone, but I won't insist on having this feature if it proves too difficult.
Before starting on this, I'd like to clarify the design requirements. ComboBoxes have the ability to "move up" and "move down" elements:
Is that what we want here? Do we want that generally for all carousels? Or generally for more layout containers? Do we want to use the visibleProperty
to determine which are included and not included? Do we like that studio UX?
11/30/22 discussion with @samreid @matthew-blackman @arouinfar
@samreid and I have been discussing this and ran into some edge cases. Currently there is a wire as the top element on every page of the carousel. We are asking about the desired behavior in the following cases:
If the user, for example, hides the fuse, do we still want a wire at the top of each page? @arouinfar
Currently the carousel has only one global 'Wire Tool Node' that is being shown at the top of each carousel page. If the user sets the wire tool node invisible, this will remove the wire tool at the top of each carousel page. Is this the intended behavior, or do we want separate wire tool nodes for each carousel page to allow more customizability? @arouinfar
This is a fun edge case. The wire appears at the top of each carousel page as a convenience since there are generally far more wires in a circuit than other element types.
If the user, for example, hides the fuse, do we still want a wire at the top of each page?
Yes, I think so. The goal is to make wires very easy to find, and keeping them in a consistent position maintains this.
If the user sets the wire tool node invisible, this will remove the wire tool at the top of each carousel page. Is this the intended behavior, or do we want separate wire tool nodes for each carousel page to allow more customizability?
I don't think we need to provide clients with the ability to decide which of the wires to keep and which to hide. I'm fine with using a global approach -- setting wireToolNode.visibleProperty
to false will hide all instances of the wire in the carousel.
@arouinfar what do you think about having the wire part of the carousel be non-scrolling? Would that be ok, or perhaps awkward and confusing?
The layout of the carousel is hard-coded and will be difficult to generalize. Especially repagination, and especially if we have sim-specific constraints like "a wire should appear at the top of every page". If we do change carousel to support removing and adding back items, can that be done solely using visibleProperty
? It seems carousels may need to sometimes leave a space for a (temporarily?) invisible icon, if we don't follow an approach like https://github.com/phetsims/circuit-construction-kit-common/issues/903. Or do we need a separate API like Carousel.setItems() or making it so each item has an optional isIncludedProperty
?
I opened a common code issue and requested advice from @pixelzoom
Some thoughts
isIncludedProperty
sounds like a good way to avoid this situation. The visibleProperty
can be read-only and controlled by the model, and we can trigger repagination if and only if isIncludedProperty
is false
.isDisposableProperty=false
. Clients can decide if a circuit element is special and nondisposable. isIncludedProperty=false
we stop repeating the wire icon, and just have the one on the first page. This loses some of the convenience factor, but maybe that's an acceptable trade-off for being able to customize the carousel contents.@arouinfar what do you think about having the wire part of the carousel be non-scrolling?
@samreid I'm trying to understand what this would look like. Would that mean that the previous button would appear below the wire? Or would the carousel would basically look the same, but the wire would simply not scroll with the rest of the contents? The latter is acceptable, the former is not. I would rather remove repeated wire icons than visually factor it out of the carousel.
EDIT: I just saw the proposal in #903, and disabling the icon when reaching the limit is also fine with me! If we go this route we may be able to use visibleProperty
instead of creating a new isIncludedProperty
.
@arouinfar please review the time estimate from @pixelzoom in https://github.com/phetsims/sun/issues/814#issuecomment-1334463348 and recommend whether this should be included in the first release or not.
@matthew-blackman and I prototyped an alternative that adds an enabledProperty
so that phet-io clients can "gray out" items in the toolbox without leaving layout holes.
For instance, I disabled the resistor like so:
We do not believe it to be a great user interface to have a bunch of disabled components that cannot be enabled, but we thought this was worth consideration in light of the extreme cost of implementing repagination and relayout for carousels.
@arouinfar please review the time estimate from @pixelzoom in https://github.com/phetsims/sun/issues/814#issuecomment-1334463348 and recommend whether this should be included in the first release or not.
My personal preference would be to invest the time into carousel because it's not going away, but I cannot make this call. I added the sun issue to the PhET-iO project board for further discussion.
@matthew-blackman and I prototyped an alternative
Thanks! I think it's really helpful to see alternative solutions.
We do not believe it to be a great user interface to have a bunch of disabled components that cannot be enabled,
I completely agree.
but we thought this was worth consideration in light of the extreme cost of implementing repagination and relayout for carousels.
I'm not a fan of the grayscale solution, but I appreciate the effort in looking into alternatives. At this point, I would prefer the empty holes to the grayscale icons. I find it less confusing.
I experimented to see if there was a simple solution to the carousel layout problem, and I developed this prototype:
It allows you to remove items from the carousel, and the layout reflows eliminating holes. You can also re-order items in the carousel the same way we reorder items in the combo box. I tested and all of this customization is preserved when launching the test sim.
Necessary steps to take this to production:
Here's a better patch that is opt-in (less disruptive for other sims) and minimal changes, and improves layout.
I committed a version that can be tested on phettest.
12/15/22 design meeting
@marlitas and I fixed the overlap/offset/clipping problem in the commit. Since the PageControl can be manually hidden, it seems the next important part of this issue would be getting rid of blank pages.
This patch is working but not yet ready to commit:
Current patch:
@matthew-blackman and @samreid can spend 30 minutes to see if there is low-hanging fruit for improvements, but we can live with things as they are now if there aren't any quick improvements to be made.
In discussions, @jbphet and @arouinfar indicated that avoiding blank pages and fixing the page control are worth more time than the 30 minutes allotted. We would also like to understand the scope of what would be necessary to make all sims have flexible layout carousels (getting rid of the opt-in flag).
This patch is working well, hides and shows dots, hides and shows pages.
This also includes the patch from https://github.com/phetsims/studio/issues/283 for ease of testing. (Select.ts only). Next steps:
Design questions:
Current patch:
Progress with @jbphet and @matthew-blackman. Function builder is getting in better shape, but we still have margin problems when not using separators.
I noticed @samreid's design questions in https://github.com/phetsims/circuit-construction-kit-common/issues/630#issuecomment-1372592176
What if there is only one page in the carousel? Should we hide the carousel arrow buttons? (they are grayed out). Should we hide the dots? Clients can easily remove them manually. So maybe that is OK.
I don't think we need to automatically hide the previous/next buttons or the remaining page control dot. Clients can do it manually if they want to.
What if you remove all elements from the carousel? Should the carousel auto-hide? But the client can already set carousel.visibleProperty to false if they want. So maybe that is OK.
No need to auto hide the entire carousel. Clients should be using carousel.visibleProperty
if that is their goal.
Fixes the margin when there are no separators:
Removes the opt-in flag:
Use alignBox so that all items are the same size:
Expression Exchange coins are not centered:
Current patch:
Number play not quite right:
Everything else seems good, including function builder.
Expression exchange got a lot better in this patch:
Number play still cuts off a bit:
This patch computes the window size by beginning of item to end of item.
When the margin matches the spacing, it is better:
Implement disposal and avoid animation during initialization:
Next up (roughly in this order):
pageNumberListener
also observes numberOfPagesProperty
Patch: Hide the buttons if there is only one page:
Recenter the dots as the number of pages changes:
Current patch, hopefully the same as the one above:
Move up/Move down on items didn't seem to work, after removing several items in studio. Could this be because it is moving the item within the alignBox instead of moving the alignBox. Also possible: separators?
@matthew-blackman and I found that the problem is wrapping each node in the Carousel. That was helpful in getting the layout right, but it interferes with IndexedNodeIO. We experimented with making IndexedNodeIO work on the grandparent index and it was mostly working OK. But would need more work to deal with separators and getting this to production. This was not a requested feature, and only added in because it was easy in the 1st draft. So here is our patch for IndexedNodeIO that works on parent index:
Current patch:
@matthew-blackman suggested we show separators in the carousel, and @arouinfar and I agreed it looks really great. @arouinfar also affirmed that it is important to be able to "move up" and "move down" even after we said it could take 2-4+ hours to implement.
I think I should move this work to branches, so we can keep making improvements there and request review before moving it to master. UPDATE: Also, it's been nice to work out details in this issue, but it seems nice to continue branches and commits in the common code issue https://github.com/phetsims/sun/issues/814, see you over there.
That commit and branch were moved to https://github.com/phetsims/sun/issues/814
Addressed in https://github.com/phetsims/sun/issues/814
OK Carousel has been merged to master. @arouinfar and/or @matthew-blackman can you please test:
Please test carousel related features such as moving item indices, removing items, dragging out all of one item, etc. Please check the tandem tree and see if anything was disrupted. It was a very complex merge and I'm having trouble telling if I got everything right.
Here are some changes in the API file: https://github.com/phetsims/phet-io-sim-specific/commit/b794abd0b8aeb6adc6c9e7b10a277d0f2d8f1472
Make it so that studio doesn't leave "holes" in the carousel during customization, but dragging out the last item should leave a hole.