-
Notifications
You must be signed in to change notification settings - Fork 356
refactor(affix): refactor affix component #3797
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors the Affix component with a cleaner implementation that improves maintainability and fixes positioning logic. The refactoring replaces a complex imperative approach with a more declarative React-based state management.
- Introduces new utility functions for calculating fixed positioning
- Refactors the component to use React hooks and state management patterns
- Updates test cases to properly verify the new implementation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/components/affix/Affix.tsx | Complete refactor of Affix component with new state management and positioning logic |
packages/components/affix/utils.ts | New utility functions for calculating fixed top and bottom positions |
packages/components/affix/_example/container.tsx | Updates example to use proper event handling pattern |
packages/components/_util/dom.ts | Adds utility function for getting target element DOMRect |
packages/components/affix/tests/affix.test.tsx | Updates tests to work with refactored implementation |
test/snap/snapshots/ssr.test.jsx.snap | Updated snapshots to include empty class attributes |
test/snap/snapshots/csr.test.jsx.snap | Updated snapshots to include empty class attributes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let top = 0; | ||
if (fixedTop !== undefined) { | ||
newState.affixStyle = { | ||
position: 'fixed', | ||
top: fixedTop, | ||
width: placeholderRect.width, | ||
height: placeholderRect.height, | ||
zIndex, | ||
}; | ||
newState.placeholderStyle = { | ||
width: placeholderRect.width, | ||
height: placeholderRect.height, | ||
}; | ||
top = fixedTop; | ||
} else if (fixedBottom !== undefined) { | ||
newState.affixStyle = { | ||
position: 'fixed', | ||
bottom: fixedBottom, | ||
width: placeholderRect.width, | ||
height: placeholderRect.height, | ||
zIndex, | ||
}; | ||
newState.placeholderStyle = { | ||
width: placeholderRect.width, | ||
height: placeholderRect.height, |
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 width and height properties are duplicated in both the fixedTop
and fixedBottom
branches. Consider extracting these common properties into a shared object to reduce duplication.
let top = 0; | |
if (fixedTop !== undefined) { | |
newState.affixStyle = { | |
position: 'fixed', | |
top: fixedTop, | |
width: placeholderRect.width, | |
height: placeholderRect.height, | |
zIndex, | |
}; | |
newState.placeholderStyle = { | |
width: placeholderRect.width, | |
height: placeholderRect.height, | |
}; | |
top = fixedTop; | |
} else if (fixedBottom !== undefined) { | |
newState.affixStyle = { | |
position: 'fixed', | |
bottom: fixedBottom, | |
width: placeholderRect.width, | |
height: placeholderRect.height, | |
zIndex, | |
}; | |
newState.placeholderStyle = { | |
width: placeholderRect.width, | |
height: placeholderRect.height, | |
const sharedRectStyle = { | |
width: placeholderRect.width, | |
height: placeholderRect.height, | |
}; | |
let top = 0; | |
if (fixedTop !== undefined) { | |
newState.affixStyle = { | |
position: 'fixed', | |
top: fixedTop, | |
...sharedRectStyle, | |
zIndex, | |
}; | |
newState.placeholderStyle = { | |
...sharedRectStyle, | |
}; | |
top = fixedTop; | |
} else if (fixedBottom !== undefined) { | |
newState.affixStyle = { | |
position: 'fixed', | |
bottom: fixedBottom, | |
...sharedRectStyle, | |
zIndex, | |
}; | |
newState.placeholderStyle = { | |
...sharedRectStyle, |
Copilot uses AI. Check for mistakes.
commit: |
🤔 这个 PR 的性质是?
🔗 相关 Issue
💡 需求背景和解决方案
重构Affix组件的实现
📝 更新日志
refactor(Affix): 重构
Affix
组件本条 PR 不需要纳入 Changelog
☑️ 请求合并前的自查清单