-
-
Notifications
You must be signed in to change notification settings - Fork 49.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
fix: Form throw error when using BraftEditor #21425
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.
Could you please add tests to make sure this change works as expected?
components/form/FormItem.tsx
Outdated
mergedControl[eventName](...args); | ||
children.props[eventName](...args); | ||
mergedControl[eventName]?.(...args); | ||
children.props[eventName]?.(...args); |
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.
BraftEditor.props.onChange
是 null。
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.
看下来是直接把 eventName in children.props
改成 children.props[eventName]
即可,这里用于把参数传递给原始的 eventHandler,没有就不用进 if 了。
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.
mergedControl 要改么
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.
这样会不会导致 BraftEditor 的 onChange 直接不触发了?
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.
不用。上面 mergeControl 已经处理过了。这里只是再额外把相关事件填充一下。
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.
triggers.forEach(eventName => {
childProps[eventName] = (...args: any[]) => {
mergedControl[eventName]?.(...args);
children.props[eventName]?.(...args);
};
});
是不是这样就行了,判断也不需要了。
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 pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov Report
@@ Coverage Diff @@
## master #21425 +/- ##
==========================================
- Coverage 97.83% 97.83% -0.01%
==========================================
Files 302 302
Lines 7017 7016 -1
Branches 1888 1887 -1
==========================================
- Hits 6865 6864 -1
Misses 152 152
Continue to review full report at Codecov.
|
component.props.onChange should not run when it is falsy close #21415
0b7717f
to
7ca3b25
Compare
🤔 This is a ...
🔗 Related issue link
close #21415
💡 Background and solution
component.props.onChange
should not run when it is falsy.📝 Changelog
☑️ Self Check before Merge