Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix: If first line in selection is a line comment, Block Comment does nothing #2121

Merged
merged 2 commits into from
Dec 5, 2012
Merged

Fix: If first line in selection is a line comment, Block Comment does nothing #2121

merged 2 commits into from
Dec 5, 2012

Conversation

TomMalbran
Copy link
Contributor

This fix would let you do a block-comment around a line-comments, if the selection includes all the content in the line-comments.

It stills has the line-uncomment for other cases.

@ghost ghost assigned redmunds Nov 15, 2012
@redmunds
Copy link
Contributor

Note that there's a typo in commit message. This is for #2118.

if (!_containsUncommented(editor, sel.start.line, sel.end.line)) {
// If the selection includes all the line-comments, do a block-comment
if (editor.posWithinRange(ctxPos, sel.start, sel.end) && (!endCtx.token.string.match(lineExp)
|| editor.indexFromPos(sel.end) >= endCtxIndex)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. My only comment would be to break this condition on the && to make logic easier to read:

if (editor.posWithinRange(ctxPos, sel.start, sel.end) &&
        (!endCtx.token.string.match(lineExp) || editor.indexFromPos(sel.end) >= endCtxIndex)) {

@TomMalbran
Copy link
Contributor Author

Yes. My mistake. The branch name and the pull name are still right.

@redmunds
Copy link
Contributor

redmunds commented Dec 5, 2012

Looks good. Merging.

redmunds added a commit that referenced this pull request Dec 5, 2012
Fix: If first line in selection is a line comment, Block Comment does nothing
@redmunds redmunds merged commit c93c170 into adobe:master Dec 5, 2012
peterflynn added a commit that referenced this pull request Dec 14, 2012
…e line

comments differently), #2133 (single-line block comment mode when Line
Comment invoked in CSS/HTML/etc. - enable test coverage), and #2148 (don't
replace whole selection - which means tests must work around #2335).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants