-
-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
refactor: affix FC rewrite #44408
refactor: affix FC rewrite #44408
Conversation
Run & review this pull request in StackBlitz Codeflow. |
|
are you still working on it ? I can continue your work if you don't have much time. |
of course, i will finish it. |
Hello, would you like to reopen PR? I've been quite busy lately. If you're willing |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feature #44408 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 686 686
Lines 11647 11635 -12
Branches 3123 3120 -3
=========================================
- Hits 11647 11635 -12
☔ View full report in Codecov by Sentry. |
ce23d3e
to
b1b5324
Compare
} | ||
this.measure(); | ||
} | ||
const status = React.useRef(AffixStatus.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.
This should all be controlled by useState
instead of useRef
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.
status
and lastAffix
are intermediate variables used in calculating the style, rather than being directly consumed by render. I think using useRef
could help avoid unnecessary re-renders.
What do you think? I'm open to discussing this further.
Please let me know if you would like me to clarify or expand on anything.
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.
@zombieJ ping
this.measure(); | ||
} | ||
const status = React.useRef(AffixStatus.None); | ||
const lastAffix = React.useRef(false); |
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 too
马上发 5.9.0 了,能顺便上车么? |
ping |
[中文版模板 / Chinese template]
🤔 This is a ...
🔗 Related issue link
close #39892
related #39900
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge
🚀 Summary
🤖 Generated by Copilot at e58e7a8
This pull request refactors the
Affix
component and its test file to use modern React features and best practices. It replaces class components with functional components and hooks, and updates the test cases to use@testing-library/react
. This improves code quality, performance, and reliability.🔍 Walkthrough
🤖 Generated by Copilot at e58e7a8
InternalAffix
class component to functional component with forward ref and use React hooks to manage state, effects, and refs (link, link, link, link, link)AffixProps
interface by removinggetInstance
prop and moveAffix
component insideindex.tsx
file (link, link, link)InternalAffixClass
from test file (link)getInstance
prop fromAffixMounter
component and spread the rest of the props toAffix
component (link)@testing-library/react
helpers and checking the presence and style of the rendered elements, instead of accessing the internal state and ref of theInternalAffix
component (link, link, link)