-
Notifications
You must be signed in to change notification settings - Fork 138
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,19 +8,22 @@ | |
import { type ComputedRef, computed } from 'vue' | ||
|
||
import { type ɵOverlayProps } from '@idux/components/_private/overlay' | ||
import { type CommonConfig } from '@idux/components/config' | ||
import { type ProSearchConfig } from '@idux/pro/config' | ||
|
||
import { type ProSearchProps } from '../types' | ||
|
||
export function useCommonOverlayProps( | ||
props: ProSearchProps, | ||
config: ProSearchConfig, | ||
componentCommonConfig: CommonConfig, | ||
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 commentThe 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. |
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 parametercomponentCommon
when calling theuseCommonOverlayProps
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 thecomponentCommon
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 theuseCommonOverlayProps
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.