Skip to content

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented May 3, 2022

This PR removes our use of ace editor's insertSnippet command. This command was causing any items which matched the token, which was by default anything starting with $ to be removed when the stage operator was changed. This unintentional replacing was made more evident when we added the ability for the stage content not to change when the operator is changed.

To fix this we are now replacing the ace snippet completion by replacing the tokens explicitly (see the replaceOperatorSnippetTokens function). As a result of this approach we are able to remove the fromStageOperators flag, which, from what it looks like, was used to prevent an additional preview reload when the stage operator changed. We're also removing the snippet from the stage model as it is no longer needed.
We could alternatively try to update how ace does its snippet completion, but I think that approach would make us more tied to ace editor internals, and might require us to keep these extra things in the state which do not need to be.

before:

before.-.token.lost.mp4

after:

token.is.kept.after.mp4

this.completer.version = this.props.serverVersion;
if (this.props.stageOperator !== prevProps.stageOperator && this.editor) {
this.editor.setValue('');
this.editor.insertSnippet(this.props.snippet || '');
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's where we used to perform the snippet completion. This would also call the onStageChange twice I believe, and by doing so cause the stage preview to be recalculated. These changes make it so we explicitly try to run now when the operator changes. This might be something we could flag in the store instead.

@Anemy Anemy merged commit 6f503ef into main May 3, 2022
@Anemy Anemy deleted the COMPASS-5683-cannot-use-dollar-sign-in-stage-operators branch May 3, 2022 20:25
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.

2 participants