-
Notifications
You must be signed in to change notification settings - Fork 140
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.
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
fix(comp:*): animating overlay are not hidden anymore #1516
Conversation
This preview will be available after the AzureCI is passed. |
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> |
There was a problem hiding this comment.
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:
- It would be better to use the enter/leave callbacks on the Transition component instead of adding a separate beforeEnter callback.
- The logic for setting the class on the overlay element should be improved to make it more concise and readable.
- The ternary operator used to conditionally render the overlay element should be refactored to use an if statement.
@@ -63,7 +63,7 @@ | |||
} | |||
} | |||
|
|||
&[data-popper-reference-hidden] { | |||
&[data-popper-reference-hidden]:not(&-animating) { | |||
visibility: hidden; | |||
opacity: 0; | |||
pointer-events: none; |
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## main #1516 +/- ##
==========================================
+ Coverage 92.75% 92.77% +0.02%
==========================================
Files 331 332 +1
Lines 30801 30892 +91
Branches 3533 3555 +22
==========================================
+ Hits 28568 28661 +93
+ Misses 2233 2231 -2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
PR Checklist
Please check if your PR fulfills the following requirements:
What is the current behavior?
正在执行动画的浮层,也会受到 reference hidden 影响
What is the new behavior?
正在执行动画的浮层不会因为 reference hidden 被隐藏
Other information