-
Notifications
You must be signed in to change notification settings - Fork 141
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(pro:search): add animation to overlay #1517
Conversation
This preview will be available after the AzureCI is passed. |
@@ -62,7 +62,7 @@ export default defineComponent({ | |||
searchItems, | |||
searchStateContext.tempSearchStateAvailable, | |||
) | |||
const commonOverlayProps = useCommonOverlayProps(props, config, mergedPrefixCls) | |||
const commonOverlayProps = useCommonOverlayProps(props, config, componentCommon, mergedPrefixCls) | |||
const focusStateContext = useFocusedState(props, elementRef, commonOverlayProps) | |||
const { focused, focus, blur } = focusStateContext | |||
|
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.
This code patch looks to be making a change to the variable commonOverlayProps
, using an additional parameter componentCommon
when calling the useCommonOverlayProps
function.
To do a proper review of this patch, I would need to see more context of the surrounding code and understand more of the logic behind the useCommonOverlayProps
function. I would also need to know what type of data is being passed in to the componentCommon
parameter and what effect this update is supposed to have on the application.
From a risk perspective, I would check that the componentCommon
parameter is not null or undefined before passing it into the function, otherwise it could cause an error. Additionally, I would check the type of value being passed in to ensure that it is compatible with the useCommonOverlayProps
function.
From an improvement perspective, I would suggest refactoring the code to make it more readable and maintainable. This could include using descriptive variable names, breaking up long lines of code, and adding comments to explain complex logic.
mergedPrefixCls: ComputedRef<string>, | ||
): ComputedRef<ɵOverlayProps> { | ||
return computed(() => ({ | ||
container: props.overlayContainer ?? config.overlayContainer, | ||
containerFallback: `.${mergedPrefixCls.value}-overlay-container`, | ||
placement: 'bottomStart', | ||
offset: [0, 8], | ||
transitionName: `${componentCommonConfig.prefixCls}-slide-auto`, | ||
offset: [0, 4], | ||
})) | ||
} |
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.
First, the code looks good and there are no bugs or issues that I can see. The only suggestion I have is to make sure that the transitionName is unique and not shared with other components. Additionally, I would suggest adding a comment explaining why the offset value was changed from 8 to 4, as this could be confusing for other developers.
Codecov Report
@@ Coverage Diff @@
## main #1517 +/- ##
==========================================
+ Coverage 92.75% 92.78% +0.03%
==========================================
Files 331 332 +1
Lines 30801 30879 +78
Branches 3533 3555 +22
==========================================
+ Hits 28568 28651 +83
+ Misses 2233 2228 -5
... 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?
之前由于浮层的trigger会在非激活状态隐藏,导致浮层触发reference hidden并且位置计算到了左上角,动画出现问题,因而去掉了动画
What is the new behavior?
以上诱发动画问题的原因已提pr修复,可以重新加回动画
Other information