-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Fontsize #888
Fontsize #888
Changes from 8 commits
6debfbf
869de18
70e802a
1a73c2b
f99a52e
883eb5a
9a5317a
a788f78
696e1d7
a4e1f63
9438517
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ | |
* | ||
*/ | ||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we prefer to explicitly call out 'window' instead of 'browser' |
||
/*global define, $ */ | ||
|
||
define(function (require, exports, module) { | ||
|
@@ -30,11 +30,75 @@ define(function (require, exports, module) { | |
var Commands = require("command/Commands"), | ||
CommandManager = require("command/CommandManager"), | ||
SidebarView = require("project/SidebarView"), | ||
ProjectManager = require("project/ProjectManager"); | ||
ProjectManager = require("project/ProjectManager"), | ||
EditorManager = require("editor/EditorManager"); | ||
|
||
function _handleHideSidebar() { | ||
SidebarView.toggleSidebar(); | ||
} | ||
|
||
CommandManager.register(Commands.VIEW_HIDE_SIDEBAR, _handleHideSidebar); | ||
/** | ||
* @private | ||
* Increases or decreases the editor's font size. | ||
* @param {string} "up" or "down" | ||
*/ | ||
function _adjustFontSize(direction) { | ||
var styleId = "codemirror-dynamic-fonts"; | ||
|
||
var fs = $(".CodeMirror-scroll").css("font-size"); | ||
var lh = $(".CodeMirror-scroll").css("line-height"); | ||
|
||
var fsUnits = fs.substring(fs.length - 2, fs.length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to guard against fs or lh having length < 2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, although we don't set other font size types like percentages, it's conceivable (though probably unlikely) that someone might set that in a theme. We might want to detect those cases and just bail rather than doing something nonsensical. (i.e., explicitly test that the last two characters are either "em" or "px", and if neither, then bail.) |
||
var lhUnits = lh.substring(lh.length - 2, lh.length); | ||
|
||
fs = fs.substring(0, fs.length - 2); | ||
lh = lh.substring(0, lh.length - 2); | ||
|
||
var fsDelta = (fsUnits === "px") ? 1 : 0.1; | ||
var lhDelta = (lhUnits === "px") ? 1 : 0.1; | ||
|
||
if (direction === "down") { | ||
fsDelta *= -1; | ||
lhDelta *= -1; | ||
} | ||
|
||
var fsStr = (parseFloat(fs) + fsDelta) + fsUnits; | ||
var lhStr = (parseFloat(lh) + lhDelta) + lhUnits; | ||
|
||
if (fsStr === "1px" || fsStr === ".1em") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we check the numeric values instead, and use <=? It's conceivable someone might set a font size like 1.25em. |
||
return; | ||
} | ||
|
||
$("#" + styleId).remove(); | ||
var style = window.document.createElement("style"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're using jQuery anyway, seems nicer to just do |
||
style.setAttribute("type", "text/css"); | ||
style.setAttribute("id", styleId); | ||
style.innerHTML = ".CodeMirror-scroll{font-size:" + fsStr + | ||
" !important;line-height:" + lhStr + " !important;}"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't seem necessary to skimp on whitespace in this string--might as well make it more legible. |
||
$("head").append(style); | ||
|
||
var fullEditor = EditorManager.getCurrentFullEditor(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following code (to the end of the function) feels like it knows too much about the guts of editors and inline editors. Could we add a new method on Editor with this code in it, called something like |
||
fullEditor._codeMirror.refresh(); | ||
|
||
var inlineEditors = fullEditor.getInlineWidgets(); | ||
inlineEditors.forEach(function (multilineEditor, i, arr) { | ||
multilineEditor.sizeInlineWidgetToContents(true); | ||
multilineEditor._updateRelatedContainer(); | ||
multilineEditor.editors.forEach(function (editor, j, arr) { | ||
editor._codeMirror.refresh(); | ||
}); | ||
}); | ||
} | ||
|
||
function _handleIncreaseFontSize() { | ||
_adjustFontSize("up"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit: could we just pass 1 or -1? |
||
} | ||
|
||
function _handleDecreaseFontSize() { | ||
_adjustFontSize("down"); | ||
} | ||
|
||
CommandManager.register(Commands.VIEW_HIDE_SIDEBAR, _handleHideSidebar); | ||
CommandManager.register(Commands.VIEW_INCREASE_FONT_SIZE, _handleIncreaseFontSize); | ||
CommandManager.register(Commands.VIEW_DECREASE_FONT_SIZE, _handleDecreaseFontSize); | ||
}); |
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.
I think this logic would be better in
_normalizeKeyDescriptorString()
, since that's actually where the logical loophole is that causes the problem--i.e., it should be treated as a workaround for the fact that we're splitting on "-", and we should explicitly test for the case there.