Fix #3478: Persistent highlight is taller than transient highlight #3568
Conversation
ghost
assigned 
| var lhStr = lhNew + lhUnits; | ||
|
|
||
| // Don't let the font size get too small or too large. | ||
| if (fsNew < MIN_FONT_SIZE * delta || fsNew > MAX_FONT_SIZE * delta) { |
There was a problem hiding this comment.
Maximum px size is 72, otherwise 7.2. This works for px and em, but if anyone changes units to anything else, the results may not be desirable. Maybe this should check for units type (like it did before).
There was a problem hiding this comment.
Nothing would actually work for other units, and that is why the code is returned on line 151 when the units aren't px or em, so this check wasn't that necessary. If a new font size unit would be introduced the whole code would need to be updated to support it, so I don't think this is an issue.
There was a problem hiding this comment.
Also, this is a bit cryptic, so please explain the 72 vs 7.2 in a comment.
The problem is keeping it the same as the line numbers. This should be a separate change. |
|
Done with initial review. |
|
@redmunds Thanks for the review. I added a comment there. The comment needs to be updated, but I don't think that the if does need to go back to how it was. Let me know if there might still be good to have it work as before.
Actually since the line numbers are now part of the code lines, the text adjustment are made to both at the same time making the em solution work. I did made a quick test in the Developer Tools. But this would also fix the problem so we don't really need to use the alternative solution. |
|
I didn't see the check for px and em at the top, so no need to check again for MIN/MAX. |
|
It's nice that line numbers are in same element as code so they can never get out-of-sync. Update the comment for min max explaining maximum size is 72px or 7.2 em and I'll merge it. |
|
@redmunds done :) |
| /** | ||
| * @const | ||
| * @private | ||
| * The proportion between the font-size and the line-height in pixels |
There was a problem hiding this comment.
I just noticed that his comment is not quite correct. The operands are reversed, and it's not in pixels (it's a ratio).
It should be "The ratio of line-height to font-size".
There was a problem hiding this comment.
Right, I intended to use it just for px, but then I realized that it would be required for ems too, when both units are ems. I'll update this comment too.
There was a problem hiding this comment.
Even better: "The ratio of line-height to font-size when font-size is in pixels"
There was a problem hiding this comment.
Actually it is used when both font-size and line-height are in px or ems, and is not when they are different. If the font-size is on px and the line-height in ems, the line height doesn't change. So the previous one is more accurate.
There was a problem hiding this comment.
It still bugs me that the operands are reversed. How about: "The ratio of line-height to font-size when the use the same units"
|
@TomMalbran 1 last comment change. |
|
@redmunds Comment updated. |
|
Merging. |
Fix #3478: Persistent highlight is taller than transient highlight
The problem in #3478 was that when the font-size increased the font-size line-height ratio was lost. They line height should always be 1.25 times higher than the font-size so that the text would fit inside the line.
I applied the solution to the font adjustment instead of the styles, to make it possible to keep using pixels for both font size and line height. If we can change the line-height to ems, an easier solution would be to just remove all the line-height adjustments in the command function.