Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Document the newly exposed 'editorKey' #1072

Closed
flarnie opened this issue Mar 15, 2017 · 4 comments
Closed

Document the newly exposed 'editorKey' #1072

flarnie opened this issue Mar 15, 2017 · 4 comments

Comments

@flarnie
Copy link
Contributor

flarnie commented Mar 15, 2017

We should more clearly document the 'editorKey', especially now that it is exposed as a prop.

Added in c0f2414

It is exposed in order to allow server side rendering - see #822 and #796 for context

Goals:

  1. Add documentation for this new prop. We would merge the PR for updated documentation when releasing v0.10.1.
  2. Add inline comments around 'editorKey' explaining the use a bit. That PR could be merged any time.

I would close this issue after those two are done. But there are some bonus items that we might discuss in the comments:

  1. We should also document a potential 'gotcha' with this prop: If the 'editorKey' is found in text that is copy-pasted then Draft will think that it is an internal paste. I have manually tried to find any bugs this would cause but it doesn't seem to interfere with copy-pasting from within or outside of the Draft editor. But let's look into this and be sure that no issues will be caused.
  2. We might also consider if there is another solution. I don't have ideas off the top of my head but it seems like there could be another way to deal with this issue. Expecting people to set the 'editorKey' prop seems really unintuitive.
@flarnie
Copy link
Contributor Author

flarnie commented Mar 15, 2017

@nsfmc this seems like something you might be interested in.

@nsfmc
Copy link
Contributor

nsfmc commented Mar 16, 2017

on it

@nsfmc
Copy link
Contributor

nsfmc commented Mar 21, 2017

re: point 4, thinking about it now after writing the docs, the main difference between the two PRs handling this was that mine used componentDidMount as the moment that the editorKey propagated to the <DraftEditorContents /> meaning that you could still get clean serverside rendering without passing through the editorKey prop at all at the expense of abusing the editor's state. The current merged behavior will always generate an editorKey in a server-render.

in writing the docs, i agree that it's weird that somebody would need to manually set the prop ever (this becomes like a very weird form of react's key prop) especially since the weird case of say, copying between two similarly keyed but unrelated editors might have unintended side-effects. That said, realizing that few people are server-rendering these components to begin with, it does feel reasonable to push the generation of editorKey off to contexts where canUseDOM would be true.

in terms of api, though, it's definitely reasonable to start with this more strict behavior and then relax the requirements than to start with them too loose and then realize that they should have been more strict from the outset. i can't think of any good ways of moving behavior to dom-only contexts other than componentDidMount+state, but if anything jumps out at you, hit me up.

@flarnie
Copy link
Contributor Author

flarnie commented Jul 7, 2017

Thanks again @nsfmc and I think we covered this, so closing this issue.

@flarnie flarnie closed this as completed Jul 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants