-
Notifications
You must be signed in to change notification settings - Fork 273
refactor: 重构引入的css transition 库 #3236
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: feat_v3.x
Are you sure you want to change the base?
Conversation
Walkthrough本次更改将 Overlay 组件中的 Changes
Sequence Diagram(s)sequenceDiagram
participant Overlay
participant CSSTransition
participant Transition
participant DOM
Overlay->>CSSTransition: 渲染时传递过渡相关 props
CSSTransition->>Transition: 包装生命周期回调与状态
Transition->>DOM: 根据状态添加/移除 CSS 类名,触发动画
Transition-->>CSSTransition: 通知过渡阶段变化
CSSTransition-->>Overlay: 过渡完成后回调
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/utils/css-transition/Transition.tsx (2)
136-137
: TODO 注释需要跟踪发现了一个关于在下个主版本中移除 fallback 的 TODO 注释。建议创建 issue 来跟踪这个技术债务。
需要我帮您创建一个 GitHub issue 来跟踪这个 TODO 吗?
54-286
: 建议改进类型安全性和可维护性这个组件管理复杂的状态转换,建议以下改进:
- 避免使用
any
类型,提高类型安全性- 考虑使用状态机库(如 XState)来更好地管理状态转换
- 将一些复杂逻辑抽取为自定义 hooks
例如,可以创建一个自定义 hook 来管理过渡状态:
const useTransitionState = (props: TransitionProps) => { const [status, setStatus] = useState<TransitionStatus>(UNMOUNTED) // ... 状态管理逻辑 return { status, updateStatus } }同时建议为状态定义类型:
type TransitionStatus = | typeof UNMOUNTED | typeof EXITED | typeof ENTERING | typeof ENTERED | typeof EXITING
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
package.json
(1 hunks)src/packages/overlay/overlay.taro.tsx
(1 hunks)src/packages/overlay/overlay.tsx
(1 hunks)src/utils/css-transition/CSSTransition.tsx
(1 hunks)src/utils/css-transition/Transition.tsx
(1 hunks)src/utils/css-transition/TransitionGroupContext.tsx
(1 hunks)src/utils/css-transition/config.ts
(1 hunks)src/utils/css-transition/index.js
(1 hunks)src/utils/css-transition/utils/PropTypes.js
(1 hunks)src/utils/css-transition/utils/reflow.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/utils/css-transition/CSSTransition.tsx (1)
src/utils/css-transition/utils/reflow.ts (1)
forceReflow
(1-1)
src/utils/css-transition/Transition.tsx (1)
src/utils/css-transition/utils/reflow.ts (1)
forceReflow
(1-1)
🔇 Additional comments (7)
src/packages/overlay/overlay.taro.tsx (1)
4-4
: 导入语句更新正确从外部库
react-transition-group
切换到本地实现,符合PR的重构目标。src/packages/overlay/overlay.tsx (1)
9-9
: 导入语句更新正确与Taro版本保持一致,成功切换到本地CSS transition实现。
src/utils/css-transition/CSSTransition.tsx (3)
1-12
: 辅助函数实现合理使用
dom-helpers
库并正确处理了多个类名的情况。
65-124
: 生命周期回调实现正确正确处理了各个过渡阶段的类名管理,并妥善处理了
nodeRef
的参数传递。
195-206
: 组件结构正确正确地将处理后的回调函数传递给底层的
Transition
组件。src/utils/css-transition/Transition.tsx (2)
1-16
: 导入和常量定义看起来不错!使用常量来定义过渡状态是良好的实践,避免了魔法字符串的使用。
18-52
: 接口定义和默认属性配置合理TransitionProps 接口定义完整,支持灵活的 timeout 配置和 children 渲染方式。默认值设置也很合理。
const addClass = (node: HTMLElement | null, type: string, phase: string) => { | ||
let className = getClassNames(type)[`${phase}ClassName`] | ||
const { doneClassName } = getClassNames('enter') | ||
|
||
if (type === 'appear' && phase === 'done' && doneClassName) { | ||
className += ` ${doneClassName}` | ||
} | ||
|
||
// This is to force a repaint, | ||
// which is necessary in order to transition styles when adding a class name. | ||
if (phase === 'active') { | ||
if (node) forceReflow(node) | ||
} | ||
|
||
if (className) { | ||
appliedClasses[type][phase] = className | ||
addClassCommon(node, className) | ||
} | ||
} |
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.
🛠️ Refactor suggestion
类名管理逻辑实现良好,但存在小问题
addClass
函数正确处理了各个阶段的类名,包括强制重排以触发过渡效果。但第156行的实现有潜在问题:
let className = getClassNames(type)[`${phase}ClassName`]
这里 phase
的值是 "base"、"active" 或 "done",但 getClassNames
返回的对象键是 baseClassName
、activeClassName
、doneClassName
。建议修正:
- let className = getClassNames(type)[`${phase}ClassName`]
+ const classNameKey = `${phase}ClassName` as keyof ReturnType<typeof getClassNames>
+ let className = getClassNames(type)[classNameKey]
🤖 Prompt for AI Agents
In src/utils/css-transition/CSSTransition.tsx around lines 155 to 173, the
variable className is assigned using getClassNames(type)[`${phase}ClassName`],
but phase values like "base", "active", or "done" need to be concatenated with
"ClassName" to match keys such as "baseClassName", "activeClassName", and
"doneClassName". Fix this by constructing the key string properly, for example
by using `${phase}ClassName` as a single string key to access the correct
property from the object returned by getClassNames.
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/css-transition/Transition.tsx (1)
225-231
:⚠️ Potential issue修复 safeSetState 函数的严重实现错误
safeSetState
函数存在严重的实现错误:
status
是字符串状态,但函数尝试将其作为对象处理并展开nextState
- 现代 React 的
setStatus
不支持回调函数作为第二个参数- 展开语法
{ ...prev, ...nextState }
对字符串状态没有意义应用以下修复:
- const safeSetState = (nextState: any, callback?: () => void) => { + const safeSetState = (nextState: { status: string }, callback?: () => void) => { // This shouldn't be necessary, but there are weird race conditions with // setState callbacks and unmounting in testing, so always make sure that // we can cancel any pending setState callbacks after we unmount. const nextCallback = setNextCallback(callback) - setStatus((prev) => ({ ...prev, ...nextState }), nextCallback) + setStatus(nextState.status) + if (nextCallback) { + // 使用 requestAnimationFrame 来模拟回调行为 + requestAnimationFrame(() => nextCallback()) + } }
🧹 Nitpick comments (2)
src/utils/css-transition/Transition.tsx (2)
120-120
: 移除生产代码中的调试语句生产代码中不应包含 console.log 语句,请移除此调试输出。
- console.log('nextStatus', nextStatus)
174-174
: 移除调试语句此处的 console.log 应该被移除,以保持代码的整洁性。
- console.log('performEnter')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json
(1 hunks)src/utils/css-transition/CSSTransition.tsx
(1 hunks)src/utils/css-transition/Transition.tsx
(1 hunks)src/utils/css-transition/TransitionGroupContext.tsx
(1 hunks)src/utils/css-transition/index.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/utils/css-transition/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/utils/css-transition/CSSTransition.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils/css-transition/Transition.tsx (1)
src/utils/css-transition/utils/reflow.ts (1)
forceReflow
(1-1)
🔇 Additional comments (4)
src/utils/css-transition/TransitionGroupContext.tsx (1)
1-11
: 实现良好,已解决之前的类型安全问题当前实现正确地定义了 TypeScript 接口和文档注释,相比之前的版本有了显著改进:
- 添加了完整的
TransitionGroupContextValue
接口定义- 为 React.createContext 提供了正确的泛型类型参数
- 包含了清晰的中文文档说明上下文的用途
这很好地解决了之前关于类型安全性的问题。
src/utils/css-transition/Transition.tsx (3)
83-101
: useEffect 依赖项已正确修复很好,这个 useEffect 的依赖项数组已经包含了所有使用的变量,解决了之前的闭包陷阱问题。
103-109
: useEffect 依赖项已正确修复依赖项数组现在正确地包含了
status
和unmountOnExit
,避免了之前的陈旧闭包问题。
111-122
: useEffect 依赖项已正确修复依赖项数组现在正确地包含了
_in
和status
,确保了状态更新的正确性。
🤔 这个变动的性质是?
🔗 相关 Issue
💡 需求背景和解决方案
☑️ 请求合并前的自查清单
Summary by CodeRabbit
新功能
依赖变更