Overwrite mode improvements #6777
Conversation
| $statusOverwrite.text(Strings.STATUSBAR_INSERT); | ||
| } else { | ||
| $statusOverwrite.text(Strings.STATUSBAR_OVERWRITE); | ||
| } |
There was a problem hiding this comment.
This:
if (!newstate) {
$statusOverwrite.text(Strings.STATUSBAR_INSERT);
} else {
$statusOverwrite.text(Strings.STATUSBAR_OVERWRITE);
}
can be simplified to:
$statusOverwrite.text(newstate ? Strings.STATUSBAR_OVERWRITE : Strings.STATUSBAR_INSERT);
|
@redmunds fixes pushed. |
| @@ -151,6 +169,7 @@ define(function (require, exports, module) { | |||
| if (!current) { | |||
| StatusBar.hide(); // calls resizeEditor() if needed | |||
| } else { | |||
| current.toggleOverwrite($statusOverwrite.text() === Strings.STATUSBAR_OVERWRITE); | |||
There was a problem hiding this comment.
Shouldn't be updating individual indicators here. This should be isolated to a function and called below (e.g. _updateLanguageInfo()).
|
Done with initial review. This works great! Only other issue is that the cursor should change to a hand (pointer) when mouse is over status bar. Once all issues have been addressed, this branch should be rebased. After I merge it, then I'll open bug against CodeMirror for |
|
@WebsiteDeveloper Maybe you missed my "Done with review" comment, so tagging you on it this time. |
|
@redmunds i absolutely missed that. |
| @@ -1381,4 +1393,4 @@ label input { | |||
|
|
|||
| .install-extension-dialog .spinner { | |||
| margin-left: 10px; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
@WebsiteDeveloper Once again, it makes reviewing code so much easier if you don't alter whitespace -- especially when it is not related to the code being changed. In this case, even the newline at the end of the file was removed. Restore whitespace that is not related to the code being changed.
There was a problem hiding this comment.
This end of file change must have happened when i fixed the rebase conflicts. And by the way it reviewing is just as easy if you append ?w=0 to the url
There was a problem hiding this comment.
Yes, I'm familiar with ?w=1, but that also hides incorrect indenting and missing newline at end of file.
|
@WebsiteDeveloper Done with review. This is looking great. 1 comment about whitespace "fixing". Could you also add a couple tests to the EditOptionHandler-test tests? I think you could use |
|
@redmunds |
|
@WebsiteDeveloper Are you using an extension that automatically updates the whitespace? If so, Brackets (as of Sprint 36) now supports project-level preferences so can you disable (or change params for) that extension in the Brackets project? |
|
I used to. But i removed it. |
I can live with it one last time :) |
|
alright |
|
@redmunds added the newline |
|
Looks great. Thanks. Merging. |
Combined #6670 and #6328 into this pull request as requested.
Waiting for review.
@redmunds