-
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
Don't use stale readOnly prop. (Fix bug #3321) #3388
Conversation
I was having a problem with editors having an initial readOnly state of true, then being changed to false, where by the editor was not editable. This has resolved that and neatly solves any issue of onDOMSelectionChange or onDOMBeforeInput having other dependencies changed in the future without similar issues arising. |
This will also provide a resolution to #3412 |
@@ -392,9 +376,25 @@ export const Editable = (props: EditableProps) => { | |||
} | |||
} | |||
}, 100), | |||
[] | |||
[readOnly] |
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 debounce for this function will somehow need to be cleared too, otherwise it's possible it memoizes and uses the stale value while it's waiting during debouncing.
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 PR will fix that it looks like, so all good if both are merged.
Thanks for fixing this readonly bug, really appreciate the contribution @kena0ki |
Is this adding or improving a feature or fixing a bug?
Bug #3321
What's the new behavior?
Even after readOnly is switched from true to false, onChange is called.
Demo after fixed:
https://issue3321-test-slate.netlify.com/examples/read-only
kena0ki@566432e#diff-3377000c0769495e84953bfcdd9bb9fc (Source code)
How does this change work?
In order for onDOMSelectionChange and onDOMBeforeInput to be updated, I added dependencies to the second argument of two useCallback hooks and two useIsomorphicLayoutEffect hooks.
Although dependencies I added to useIsomorphicLayoutEffect hooks is onDOMSelectionChange and onDOMBeforeInput, an undefined variable error occurs if they are at the current location. So I moved them down below where they are defined.
Have you checked that...?
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)Does this fix any issues or need any specific reviewers?
Fixes: #3321
Reviewers: @ianstormtaylor