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

Change <Slate> to a controlled component #3216

Merged
merged 3 commits into from
Dec 5, 2019
Merged

Conversation

ianstormtaylor
Copy link
Owner

@ianstormtaylor ianstormtaylor commented Dec 3, 2019

Is this adding or improving a feature or fixing a bug?

Improvement.

What's the new behavior?

It changes <Slate> to be a controlled component.

How does this change work?

It takes value= and selection= props and ensures they are updated on the editor whenever they change, such that people can use their own useState to store them. People must now store state for both of these.

Currently it is:

<Slate defaultValue={initialValue}>
  // ...
</Slate>

With this PR it is:

const [value, setValue] = useState(initialValue)
const [selection, setSelection] = useState(null)

<Slate 
  value={value} 
  selection={selection} 
  onChange={(value, selection) => {
    setValue(value)
    setSelection(selection)
  }}
>
  // ...
</Slate>

For an example in context, check out the changes to the rich-text example on this branch.

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: #3209

@ianstormtaylor ianstormtaylor changed the title Fix controlled Change <Slate> to a controlled component Dec 3, 2019
@PeterKottas
Copy link

Looks great, well done man!

@@ -0,0 +1,41 @@
import React, { useMemo } from 'react'
import { Editor, Node, Operation, Range } from 'slate'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving this to a different module was a good decision i think, it took a little bit to find it just diving through github.

editor.selection = selection
return [editor]
}, [value, selection, ...Object.values(rest)])

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if there will be some gotchas around memoizing around the extra props. for the most part this is probably what a caller is going to need but this seems like a subtle trap for users who are not paying attention

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, although by including them it's less restrictive (not more) so I think it's alright? Otherwise implementing your own custom editor properties becomes impossible I think?

@cameracker
Copy link
Collaborator

This looks pretty good to me. I'm happy that this still works well with the context based distribution work flow.

@focux
Copy link

focux commented Dec 4, 2019

Looks good to me, also, thank you for all your work @ianstormtaylor.

@benjycui
Copy link
Contributor

benjycui commented Dec 4, 2019

How can I get operations for collaborative editing after this MR?

@wmertens
Copy link
Contributor

wmertens commented Dec 4, 2019

I would prefer that the entire state gets controlled as state and the current value is a part of that (much like it was before).

  • plugins can add their own editor state without changing the public API. E.g. a history plugin can add a history key to the state.
  • only one state needs storing by the user
  • the name state makes clear that it's not just the text value

@rikkit
Copy link

rikkit commented Dec 4, 2019

Not really been following this - is it possible to use Slate as an uncontrolled component after this? Or is this fully switching it over? I guess you can always have a wrapper component to make it uncontrolled.

@Jokcy
Copy link
Contributor

Jokcy commented Dec 4, 2019

@ianstormtaylor https://docs.slatejs.org/concepts/xx-migrating#plugins

So plugin deps on operation won't work anymore? What's the new way to build a plugin?

@PeterKottas
Copy link

PeterKottas commented Dec 4, 2019

I think your question doesn't belong here @Jokcy, try slack, this PR is for making slate into a controlled component again.

@ianstormtaylor , just out of curiosity, since this is a breaking change, how long do you think it will take to get this out? Not sure what the policy about stuff like this is with slate. Thanks!

@ianstormtaylor
Copy link
Owner Author

How can I get operations for collaborative editing after this MR?

You can access them from editor.operations. (Actually you can access the children and selection that way too, but I've changed the onChange handler to be more of a convenience for making things React-ish instead of "the only way to get things".)

@ianstormtaylor
Copy link
Owner Author

Not really been following this - is it possible to use Slate as an uncontrolled component after this? Or is this fully switching it over? I guess you can always have a wrapper component to make it uncontrolled.

This fully switches to a controlled component. I think using React's own helpers you can make it "uncontrolled" kind of. But I'd rather have only a single way to do it.

@ianstormtaylor ianstormtaylor merged commit f3fc2c2 into master Dec 5, 2019
@ianstormtaylor ianstormtaylor deleted the fix-controlled branch December 5, 2019 16:36
@maksimsemenov
Copy link

I think docs still refer to the old API (an un-controlled). Not sure if it is a known issue, though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Api for setting value from an external change
9 participants