-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat/#5521/derive on selection change and on value change #5523
feat/#5521/derive on selection change and on value change #5523
Conversation
@zbeyens let me know if it fills the expectations or there's anything that needs to be changed 😄 |
@@ -366,6 +374,14 @@ export const withReact = <T extends BaseEditor>( | |||
onContextChange() |
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.
You need to pass a parameter to this function as stated in #5521 (comment)
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.
I'm not getting what you mean. What should I pass as parameter here?
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.
We need 2 new props to Slate
: onSelectionChange
and onValueChange
. These callbacks should be called in onContextChange
like onChange
.
Could you add unit tests for:
|
Where should I add the tests? |
In |
@@ -23,6 +23,7 @@ export const Slate = (props: { | |||
children: React.ReactNode | |||
onChange?: (value: Descendant[]) => void | |||
}) => { | |||
// do we need to add onValueChange and onSelectorChange here? |
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.
here is where I need to add the new handlers, @zbeyens?
and execute them inside onContextChange?
or do you want me to add a new parameter on onContextChange, where we'll send the onValueChange
/onSelectorChange
(from this line)
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.
Whatever that makes it work :) Will be confirmed by unit tests.
Another contributor has submitted a complete and satisfactory PR #5526. Therefore, this one can be closed. Thank you for your efforts. |
Description
Keep onChange and add onSelectionChange to listen to selection change only and onValueChange to listen to value change only.
Issue
Fixes: #5521
/claim udecode/plate#2700
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)cc: @zbeyens