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

editor.setEditable should be allowed to omit update. #3289

Closed
ZaymonFC opened this issue Oct 9, 2022 · 5 comments
Closed

editor.setEditable should be allowed to omit update. #3289

ZaymonFC opened this issue Oct 9, 2022 · 5 comments
Labels
Info: Stale The issue or pullrequest has not been updated in a while and might be stale Type: Feature The issue or pullrequest is a new feature

Comments

@ZaymonFC
Copy link
Contributor

ZaymonFC commented Oct 9, 2022

What problem are you facing?

After #3140 some behaviour broke in my app because I wasn't expecting the command to trigger onUpdate().

What’s the solution you would like to see?

I'd like an optional parameter emitUpdate similar to the API provided by editor.commands.setContent:

export const setContent: RawCommands['setContent'] = (content, emitUpdate = false, parseOptions = {}) => ({ tr, editor, dispatch }) => {

The parameter would gate the event emission here:

public setEditable(editable: boolean): void {
this.setOptions({ editable })
this.emit('update', { editor: this, transaction: this.state.tr })
}

What alternatives did you consider?

Maybe there's a way to look at the editor state or transaction state in the onUpdate({editor, transaction}) => ... callback to know when the isEditable property has been flipped but I couldn't think of anything.

Anything to add? (optional)

I'm happy to implement this PR if you all don't see any issues with this. Adding an optional parameter is a non-breaking change and the default value should be true.

@ZaymonFC ZaymonFC added the Type: Feature The issue or pullrequest is a new feature label Oct 9, 2022
@dominikklein
Copy link

I can agree that something like this is really needed, struggled yesterday about the same situation. The editable flag is for me more a read-only, so the value still exists and I was really confused that the onUpdate is triggered.

@Rhys-T
Copy link
Contributor

Rhys-T commented Oct 27, 2022

Would it make sense for setEditable to have its own event type, rather than triggering update? That way, node views could always be sure that they were correctly following the isEditable status of the editor, but unrelated uses of onUpdate wouldn't get triggered unnecessarily. (On the other hand, that would be a breaking change for any code that's currently using onUpdate to watch isEditable…)

@ZaymonFC
Copy link
Contributor Author

ZaymonFC commented Nov 3, 2022

@Rhys-T Breaking an existing contract for the added functionality probably isn't worth it. Consuming code won't be affected by the suggested change and it's lower touch. If there's a case for what you are suggesting I think it would warrant it's own issue and discussion.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

This issue is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Info: Stale The issue or pullrequest has not been updated in a while and might be stale label Feb 2, 2023
@ZaymonFC
Copy link
Contributor Author

ZaymonFC commented Feb 5, 2023

Resolved with the merge of #3301

@ZaymonFC ZaymonFC closed this as completed Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Info: Stale The issue or pullrequest has not been updated in a while and might be stale Type: Feature The issue or pullrequest is a new feature
Projects
None yet
Development

No branches or pull requests

3 participants