Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(comp:*): animating overlay are not hidden anymore #1516

Merged
merged 1 commit into from
Mar 30, 2023
Merged

fix(comp:*): animating overlay are not hidden anymore #1516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions packages/components/_private/overlay/src/Overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { isFunction } from 'lodash-es'
import { vClickOutside } from '@idux/cdk/click-outside'
import { type PopperElement, type PopperEvents, type PopperOptions, usePopper } from '@idux/cdk/popper'
import { CdkPortal } from '@idux/cdk/portal'
import { Logger, callEmit, convertElement, getFirstValidNode } from '@idux/cdk/utils'
import { Logger, callEmit, convertElement, getFirstValidNode, useState } from '@idux/cdk/utils'
import { useGlobalConfig } from '@idux/components/config'
import { useZIndex } from '@idux/components/utils'

Expand Down Expand Up @@ -88,7 +88,12 @@ export default defineComponent({
{ immediate: true },
)

const [isAnimating, setIsAnimating] = useState(false)
const onBeforeEnter = () => {
setIsAnimating(true)
}
const onAfterLeave = () => {
setIsAnimating(false)
if (props.destroyOnHide) {
destroy()
}
Expand Down Expand Up @@ -131,6 +136,7 @@ export default defineComponent({
props,
mergedPrefixCls,
visibility,
isAnimating,
currentZIndex,
contentNode!,
contentArrowRef,
Expand All @@ -143,7 +149,7 @@ export default defineComponent({
<>
{trigger}
<CdkPortal target={mergedContainer.value} load={visibility.value}>
<Transition appear name={props.transitionName} onAfterLeave={onAfterLeave}>
<Transition appear name={props.transitionName} onBeforeEnter={onBeforeEnter} onAfterLeave={onAfterLeave}>
{content}
</Transition>
</CdkPortal>
Expand Down Expand Up @@ -173,6 +179,7 @@ function renderContent(
props: OverlayProps,
mergedPrefixCls: ComputedRef<string>,
visibility: ComputedRef<boolean>,
isAnimating: ComputedRef<boolean>,
currentZIndex: ComputedRef<number>,
contentNode: VNode[],
arrowRef: Ref<PopperElement | undefined>,
Expand All @@ -183,12 +190,18 @@ function renderContent(
if (props.destroyOnHide && !visibility.value) {
return null
}

const prefixCls = mergedPrefixCls.value
const classes = {
[prefixCls]: true,
[`${prefixCls}-animating`]: isAnimating.value,
}

const { triggerId } = props
const overlayId = triggerId != null ? `__IDUX_OVERLAY-${triggerId}` : undefined
const style = `z-index: ${currentZIndex.value}`
const overlay = (
<div ref={popperRef} id={overlayId} class={prefixCls} style={style} {...popperEvents.value} {...attrs}>
<div ref={popperRef} id={overlayId} class={classes} style={style} {...popperEvents.value} {...attrs}>
{contentNode}
{props.showArrow && <div ref={arrowRef} class={`${prefixCls}-arrow`}></div>}
</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a review on the code patch.

The code patch adds a new state variable, isAnimating, and uses it to conditionally add an animation class to the overlay element. It also adds a beforeEnter callback to the Transition component which updates the isAnimating state.

Overall, the patch looks fine. However, there are a few things that could be improved:

  1. It would be better to use the enter/leave callbacks on the Transition component instead of adding a separate beforeEnter callback.
  2. The logic for setting the class on the overlay element should be improved to make it more concise and readable.
  3. The ternary operator used to conditionally render the overlay element should be refactored to use an if statement.

Expand Down
2 changes: 1 addition & 1 deletion packages/components/_private/overlay/style/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
}
}

&[data-popper-reference-hidden] {
&[data-popper-reference-hidden]:not(&-animating) {
visibility: hidden;
opacity: 0;
pointer-events: none;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

The patch of code looks like it is trying to hide a data popper reference while not animating.The syntax looks correct, however, it is important to check if the data popper reference is being hidden as expected when not animating. It would be beneficial to add a unit test to ensure that the data popper reference is properly hidden when not animating. If necessary, more tests can be added to cover other scenarios and test for regressions. Additionally, it would be beneficial to refactor the code to make sure that it is more readable and easier to maintain.

Expand Down