Fix Issue #7283: Padding with line numbers turned off is broken #7641
Conversation
|
@TomMalbran, hold up I added too much padding with the line numbers. Working on it now. |
|
Yep. I noticed. The class should only be applied if the line numbers are hidden, probably only once after the html is ready from the EditorOptionHandlers |
|
@TomMalbran, okay, ready now. |
| @@ -91,6 +91,10 @@ define(function (require, exports, module) { | |||
| _.each(_optionMapping, function (commandName, prefName) { | |||
| CommandManager.get(commandName).setChecked(PreferencesManager.get(prefName)); | |||
| }); | |||
|
|
|||
| if (!PreferencesManager.get(SHOW_LINE_NUMBERS)) { | |||
There was a problem hiding this comment.
You can use Editor.getShowLineNumbers() here
|
One final thing. Does the padding seems enough for you, with show line numbers off? It seems to me that a bit larger might look better. cc @larz0 in case you want to take a look. I guess I should assign this PR to myself. jeje |
|
@TomMalbran, if we go strictly off of the original bug, any padding that keeps the selected file arrow head from overlapping on the editor text is sufficient. If I go off of my own needs, any padding allows me to print my ruler's zero tick mark in full is sufficient. By both of these criteria, the padding is large enough. But yeah, let's see what @larz0 has to say as well. |
|
For usability is large enough, but from a design point of view it looks too close to the border or to the arrow. But it might also be just me used to have the line numbers to create extra spacing. |
|
@TomMalbran, code review changes committed. It's up to you if you want to merge now or wait on input from @larz0 on the padding width. |
|
The code looks great, and actually we are using the same padding as before it was broken, so I think that it is fine. The is one other issue I just realized. Issue #7283 came after fixing #5293 by using a darker color for the line numbers. With this fix, that issue would be broken in the case of no line numbers, but this could be solved using a box-shadow: That code is working nice for me. |
|
@TomMalbran if we take into account inline editors though, it seems like we need more changes than this. There is the color for active main editor, inactive main editor, active inline editor, and inactive inline editor. |
|
You are right. But we might be able to do all of that in just CSS. The alternative would be to just have an empty gutter and not need to deal with any CSS. I am not sure which one is easier at this point. |
|
Sublime Text behaves the way you are suggesting as well. I will play around with it some more. I will make sure that the color of the padding is consistent with the line numbers shown or hidden. |
|
@TomMalbran, code review changes committed. |
|
Hold on, one more fix I have to do. |
|
@TomMalbran, okay, that covers all of the permutations. I'm not sure it's the best way to do the styles, the inline editor style classes are a bit confusing to me. Let me know if you think there is a cleaner way to do this since you have some experience with these styles. |
| @@ -247,6 +258,14 @@ | |||
| .CodeMirror-matchingtag { background: @matching-bracket; } | |||
| } | |||
|
|
|||
| .show-line-padding .inline-text-editor .CodeMirror-activeline-background { | |||
There was a problem hiding this comment.
This is working great, but we never used .inline-text-editor, so just to be more consistent with the other classes, we could move this new 2 styles to line 258 and change them to:
.show-line-padding & .CodeMirror .CodeMirror-activeline-background {
box-shadow: none;
}
.show-line-padding & .CodeMirror-focused .CodeMirror-activeline-background {
box-shadow: inset @code-padding 0 0 0 @tc-gray-panel-top-bar;
}
There was a problem hiding this comment.
@TomMalbran, I agree that for consistency sake, your suggestion is correct. I will update soon.
Just to express my opinion, though, I think using .inline-text-editor in the rules is much easier to read and understand than the nested, double .CodeMirror hack we are currently using.
There was a problem hiding this comment.
I didn't knew about that class until now. It seems nice but we need to be sure that everything works fine after using it. Maybe you can open a new issue for that, or discuss it in the forums.
|
@TomMalbran, Code Review changes committed. |
|
This looks great. This wasn't as easy as expected, jeje |
Fix Issue #7283: Padding with line numbers turned off is broken
Fix Issue #7283: Padding with line numbers turned off is broken.