refactor: add enhanced useOpenClose controller to phase out toggleOpenClose#14040
refactor: add enhanced useOpenClose controller to phase out toggleOpenClose#14040
Conversation
Elijbet
changed the title
|
This is an alternative solution to refactor: add useOpenClose controller#11309
|
Elijbet
force-pushed
the
elijbet/useOpenCloseController
branch
from
53a0dcd to
4a042aa
Compare
github-actions
bot
added
the
refactor
| onClose: () => void; | ||
| transitionEl?: HTMLElement; | ||
| transitionRef?: Ref<HTMLElement>; | ||
| updateComplete: ReactiveElement["updateComplete"]; |
There was a problem hiding this comment.
Actually, do you need this at all? LitElement already extends ReactiveElement it seems
| }); | ||
|
|
||
| function getOpenState(host: UseOpenCloseComponent): boolean { | ||
| return visibilityProps.every((visibilityProp) => getOpenStateForProp(host, visibilityProp)); |
There was a problem hiding this comment.
@Elijbet these would need to be separate states. open/expanded/closed/collapsed are all different states that should be managed separately.
There was a problem hiding this comment.
Components shouldn't have both open and closed nor should they have expanded and collapsed but they could have open and expanded or open and collapsed. etc.
| import type { KebabCase } from "type-fest"; | ||
| import { ReactiveElement } from "lit"; | ||
| import { whenTransitionDone } from "../utils/dom"; | ||
|
|
There was a problem hiding this comment.
@Elijbet we may want to introduce mutually exclusive state types:
type ExclusiveState<A extends string, B extends string> =
| ({ [K in A]?: boolean } & { [K in B]?: never })
| ({ [K in B]?: boolean } & { [K in A]?: never });
export type OpenCloseState = ExclusiveState<"open", "closed">;
export type ExpandedCollapseState = ExclusiveState<"expanded", "collapsed">;
export type OpenCloseExpandedCollapseState = OpenCloseState & ExpandedCollapseState;New exported types:
OpenCloseState: open or closed (not both)
ExpandedCollapseState: expanded or collapsed (not both)
OpenCloseExpandedCollapseState: combines both constraints
| /** | ||
| * Interface for components using the open/close controller. | ||
| */ | ||
| export type UseOpenCloseComponent = LitElement & |
There was a problem hiding this comment.
If this isn't used externally, let's not export it for now.
| controller.onUpdate(() => { | ||
| const currentOpenState = getOpenState(component); | ||
|
|
||
| if (previousOpenState !== currentOpenState) { |
There was a problem hiding this comment.
Can you look at all components using toggleOpenClose and see whether this can be applied only when the open prop changes in willUpdate/onUpdate? Some components also consider additional props (e.g., disabled) when deciding whether to call the util.
| * Controller for managing open/close lifecycle and transitions. | ||
| */ | ||
| export const useOpenClose = <T extends UseOpenCloseComponent>( | ||
| visibilityProps: VisibilityPropList, |
There was a problem hiding this comment.
To future-proof, can you refactor the params to be an object of options instead of a single option?
| }; | ||
|
|
||
| /** | ||
| * Controller for managing open/close lifecycle and transitions. |
There was a problem hiding this comment.
This is mainly used for open/close-related events.
| export type UseOpenCloseComponent = LitElement & | ||
| VisibilityState & { | ||
| transitionProp?: KebabCase<Extract<keyof CSSStyleDeclaration, string>>; | ||
| onBeforeOpen: () => void; |
There was a problem hiding this comment.
Can you refactor the event hooks so they’re specified via the controller options? This would help keep controller related props together and discourage calling the methods separately.
| ({ open: boolean } | { closed: boolean } | { expanded: boolean } | { collapsed: boolean }); | ||
|
|
||
| /** | ||
| * Interface for components using the open/close controller. |
There was a problem hiding this comment.
Suggestion: Interface for open/close event-emitting components.
| return !host.collapsed; | ||
| } | ||
|
|
||
| async function handleOpenClose(host: UseOpenCloseComponent, isOpen: boolean): Promise<void> { |
There was a problem hiding this comment.
Can you rename this to better express the intent? On first read, it’s not obvious what it does.
|
|
||
| const openingControlledPromise = createControlledPromise<void>(); | ||
| const getAnimationsSpy = vi.spyOn(component.transitionEl, "getAnimations"); | ||
| getAnimationsSpy.mockImplementation(() => [ |
There was a problem hiding this comment.
Could you see if real animations can be used? openCloseComponent.browser.spec.tsx had to mock because they were not available in the environment.
| return visibilityProps.every((visibilityProp) => getOpenStateForProp(host, visibilityProp)); | ||
| } | ||
|
|
||
| function getOpenStateForProp(host: UseOpenCloseComponent, visibilityProp: VisibilityProp): boolean { |
There was a problem hiding this comment.
This might need to be configured per component via the controller options. For example, dialog uses opened, which is internal and not covered here.
3 participants
Related Issue: #11305
Summary
Add enhanced
useOpenClosecontroller to phase outtoggleOpenClose.