fix(markdown): Fix code block selection off by one error #393
Conversation
alizaberger
added
bug
✅ Deploy Preview for stacks-editor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| const from = state.selection.from; | ||
| let to = state.selection.to; | ||
|
|
||
| // check if we need to unwrap |
There was a problem hiding this comment.
Is already done in the calling function
threefjefff
left a comment
threefjefff
left a comment
There was a problem hiding this comment.
Code looks good, tests look good, some potential for additional cleanup.
src/commonmark/commands.ts
Outdated
| to += (formattingText.length * 2) + preceedingNewlineNeeded.length; | ||
|
|
||
| // set the selection to the text inside the code fences, skipping the leading newline | ||
| if (dispatch) { |
There was a problem hiding this comment.
While we're editing code here, can we also do a check for is dispatch is defined earlier in the code? We can skip a lot of pointless transaction building if we establish early whether or not this action is valid, and that seems possible by the time we've got to let tr = state.tr;
From here:
To be able to query whether a command is applicable for a given state, without actually executing it, the dispatch argument is optional—commands should simply return true without doing anything when they are applicable but no dispatch argument is given
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
I tested and this works well. It's nice to see this bug get fixed while simplifying the code.
Only minor suggestion is to give npm run format which will resolve the CI lint failure. I'll get you an approval asap once the pipeline succeeds.
Other than that, this seems good to go IMO! Thanks for the bug fix
Describe your changes
Fixes the issue described in this meta post. In markdown mode, the code block button surrounds one character extra than the user's selection. This PR corrects that, cleans up the code, and adds comments to make it more understandable.
PR Checklist
/** ... */docsbug/enhancementand other labels as appropriateEnvironment(s) tested