fix(pro:search): add animation to overlay #1517
Conversation
|
This preview will be available after the AzureCI is passed. |
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| @@ -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.
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.
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