-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
I expected this feature to work the same as line commenting with // in a JavaScript file, so this does not works as I expected it to in these cases:
In a JS file results in this:
So I would expect a CSS file to be commented like this:
The result I get is what I wold expect from the block commenting feature:
I don't see the purpose of this feature if it's going to do the same as block commenting. |
* | ||
* The implementation uses blockCommentPrefixSuffix, with the exception of the case where | ||
* there is no selection on a uncommented and not empty line. In this case the whole lines gets | ||
* commented in a bock-comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: bock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: fix this typo
I could see it both ways. The current implementation here is consistent with Sublime, so it can't be that bad :-) @redmunds, re "what's the point?" -- to me part of the motivation for this is just discoverability and muscle memory. If you hit the shortcut you're used to using for comments in JS and it does nothing in a CSS file, it feels a little broken. But there is also a difference in behavior: if you have nothing selected, the block-comment command inserts an empty comment at the cursor position instead of commenting the whole line. |
@peterflynn I think that along with muscle memory comes expectation of similar behavior. To extend the case above, I think a result of:
is OK. The expectation for me is more about what gets commented than how it gets commented. The other case I forgot to mention is a range selected in the middle of a line:
results in:
where I expected:
I think this would be easy to implement with existing code, so I guess it's just a matter of deciding on expected behavior. |
Done with initial review. |
I was thinking of doing something that would make the line-comment, comment the complete lines in the selection, since for block-commenting there is the other function. But I went with Sublime's approach in this pull. This different behavior seems easy enough to implement, since the only hard part of restoring the original selection should always do the same thing. |
Tom, Sorry I didn't see your updates. Be sure to add a comment after you push changes to trigger an e-mail to get the attention of the reviewer. Anyway, we're trying to close down sprint 17, so I won't get to this until maybe next week. Thanks, Randy |
@redmunds Its ok. I did figure you were all too busy finishing the main features for this sprint, and this can wait. But i didn't knew about the mail trigger, will do it next time. Thanks |
selEnd = sel.end, | ||
prefixExp = new RegExp("^" + StringUtils.regexEscape(prefix), "g"), | ||
lineSelection = sel.start.ch === 0 && sel.end.ch === 0 && sel.start.line !== sel.end.line, | ||
multipleLine = sel.start.line !== sel.end.line, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change these variable names to something like isLineSelection
and isMultipleLine
to indicate that they are boolean values?
Done with review. This works great! Just a couple minor comments. |
Thanks. Fixed the tipo, the variables naming and the if nesting. |
Looks good. Thanks. Merging. |
Adding line comment to CSS and HTML as suggested here: #2119
The implementation is like Sublime's where the entire line (avoiding empty spaces at the start) gets commented if there is no selection, and just does a block-comment if there is a selection.