Skip to content
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

Merged
merged 1 commit into from
Feb 22, 2020

Conversation

kena0ki
Copy link
Contributor

@kena0ki kena0ki commented Dec 25, 2019

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...?

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)

Does this fix any issues or need any specific reviewers?

Fixes: #3321
Reviewers: @ianstormtaylor

@Jmaharman
Copy link

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.

Jmaharman added a commit to Jmaharman/slate that referenced this pull request Jan 13, 2020
@ryanmitts
Copy link
Contributor

This will also provide a resolution to #3412

@@ -392,9 +376,25 @@ export const Editable = (props: EditableProps) => {
}
}
}, 100),
[]
[readOnly]
Copy link
Contributor

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.

Copy link
Contributor

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.

@cameracker
Copy link
Collaborator

Thanks for fixing this readonly bug, really appreciate the contribution @kena0ki

@cameracker cameracker merged commit 1f11276 into ianstormtaylor:master Feb 22, 2020
thallada pushed a commit to considerhq/slate that referenced this pull request Feb 26, 2020
pzhine pushed a commit to databyss-org/slate that referenced this pull request May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggling readOnly prop from true to false breaks the Editor
4 participants