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

Controlled component #180

Closed
Yomguithereal opened this issue Jan 21, 2019 · 11 comments · Fixed by #207
Closed

Controlled component #180

Yomguithereal opened this issue Jan 21, 2019 · 11 comments · Fixed by #207
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@Yomguithereal
Copy link

Hello draftail team,

Sorry if I did not found the option in the docs but I was just wondering whether you would add the possibility to handle a Draftail editor as any controlled react input (what's more, draft.js normally works this way). I can see the appeal of having a debounced onSave hook but the fact that one cannot control the editor using react unidirectional data flow make it hard to use with usual patterns. What's more, can I help you in any way to add this, if you want to?

@thibaudcolas thibaudcolas added enhancement New feature or request help wanted Extra attention is needed labels Jan 23, 2019
@thibaudcolas thibaudcolas added this to the Nice to have milestone Jan 23, 2019
@thibaudcolas
Copy link
Collaborator

Hey @Yomguithereal 👋

It's indeed not possible to use the editor as a controlled component at the moment. To be honest I'm not sure there is a good reason for this – it's because I didn't need it back then, and because the Draft.js distinction between the EditorState and the serialisable ContentState didn't seem to fit well with the controlled component’s input/output model.

That being said, yep, if you find it useful and we can think of a good way to implement this in a backwards-compatible way then it would be great to have this option. Can you think of examples of reusable form components that offer both a controlled and uncontrolled API, that we could learn from?


Without diving into the details I could see two ways to do it:

  • Have <DraftailEditor/> support either rawContentState + onSave, or editorState + onChange.
  • Expose both a <DraftailEditor/> and <ControlledDraftailEditor/>, with the former using the latter under the hood.

Alternatively, I've been wanting to refactor most of the editor’s functionality as a draft-js-plugin, or multiple ones (#83). This is a much bigger undertaking but it would solve this API problem.

@Yomguithereal
Copy link
Author

Hello @thibaudcolas.

Can you think of examples of reusable form components that offer both a controlled and uncontrolled API, that we could learn from?

I don't have examples of this in mind. I will search nonetheless.

Personally, I would vouch for:

Have support either rawContentState + onSave, or editorState + onChange.

What's more, I have the intuition that <ControlledDraftailEditor/> would be implemented on top of this implementation anyway. But I do agree it would be a bit awkward for a component to have both behaviors. But I guess the controlled props can be documented in the advanced sections to mitigate this.

@thibaudcolas
Copy link
Collaborator

👌 In my mind I would see ControlledDraftailEditor as having the editorState and onChange props, then DraftailEditor would expose the same API as it does at the moment, but under the hood use ControlledDraftailEditor.

Thinking through this further, I'm concerned about the way Draft.js handles decorators, stored in the editor state. This means that either Draftail will need to set the decorators on the editorState passed to it, like draft-js-plugin does (https://github.com/draft-js-plugins/draft-js-plugins/blob/1da9943359a7e3dd9076daef2b4bea9de0e34eae/draft-js-plugins-editor/src/Editor/index.js#L72-L93t), or the state container needs to be aware to set the decorators itself on the editorState like Draftail would. Either way it's not very nice.

Could you say more about your use case? While I can see the value in having an onChange prop instead of the debounced callback, it's not super clear to me what would be the value in having access (and storing) the full editorState to initialise Draftail with it.

@Yomguithereal
Copy link
Author

Yomguithereal commented Jan 23, 2019

Sure. My use case is that I am building a custom CMS and I am using draftail as the rich content editor and I have the following issue:

I have an edition panel, that can either be in edition mode or preview mode. The rich content is mapped to component state and is persisted when saved in the server. But, when I switch to preview mode, the editor is not rendered anymore (which is normal), but when I come back to edition mode, then I need to pass the initial content state, which has changed. So I need this kind of gimmick here to store the initial content in a class property and refresh it when swiching modes, since I cannot provide my state data as the value.

Tell me if I can elaborate a bit more. I know it's a bit convoluted :).

@strizzaflex
Copy link

I have run into two use cases so far.

  1. Clearing the editor has come up where I work but not something I have come up against personally.

  2. Re-loading the content of the editor after it has been constructed. My case is allowing the user to upload a file and we will parse that file and convert it into html or plain text and then convert to raw.

I'm not sure that this needs to be a controlled component for this to work. Doing something like a static function that returns an observable of [string, rawconent] would be fine and expose on the service a way to push next to the observable. The idea would be that the string is the key of the editor and the content is the content for that editor that way one service can serve many editors and the editor just filters the observable based on it's key. If the key isn't set they can't use the new service.

I think doing it this way would be a non breaking change. I haven't looked that close at things just yet. For now I'm going to just change the editor key and force a re-render that way.

Let me know how you feel about this approach.

@thibaudcolas
Copy link
Collaborator

@strizzaflex that approach seems like it wouldn't even require changes to the library? It might not be the most eloquent of APIs though.

The other side of the coin is to have an onChange callback. I don't think this would be a breaking change, so if someone wants to go ahead and make a PR it's very welcome.

@almeidarruben
Copy link

almeidarruben commented May 23, 2019

@thibaudcolas in my case, I have a pre-filled form that can receive updates, so I need to control the editor contents.

Since this problem only came up today and needed to be solved, I have a working solution.

I can open a PR and we can see how we can solve this?

@thibaudcolas
Copy link
Collaborator

Hey @almeidarruben, can you explain how you’ve implemented this? I’d gladly accept a PR, but I have a pretty clear idea of how I would like this to work – so either your solution matches what I have in mind and that’s great, otherwise you can convince me that your solution is better without spending time implementing a PR.

@thibaudcolas
Copy link
Collaborator

Looking into this further, I’d like to implement this as:

  • Two new props on DraftailEditor, editorState and onChange.
  • onChange is called instead of setState in the editor’s onChange, with the filtered editor state as a parameter.
  • If onChange is set, onSave is never called.
  • editorState is the value that the editor renders, effectively replacing this.state.editorState.

I’m not entirely sure whether the existing code should be refactored so editor state is accessed from the props rather than state, or if it’s better to store it in the state when receiving a new prop (https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops). The former is probs more performant.

For my initial concern about decorators, they are already added separately because of how draft-js-plugins works so this should be fine.

And in terms of API, React has value and defaultValue on its DOM form elements for a similar purpose to this so it seems ok to me that both onChange / onSave and rawContentState / editorState both live on the same component, without being both usable at the same time.

Pull request(s) welcome, especially starting with onChange since it seems like it’s a much more straightforward change!

@thibaudcolas
Copy link
Collaborator

This is now released in Draftail v1.3.0 – https://www.draftail.org/blog/2019/08/15/draftail-v1-3-0-community-improvements-beyond-wagtail. Please try it out and let me know what you think!

@Yomguithereal
Copy link
Author

Thanks @thibaudcolas. Will try this soon in our custom CMS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants