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

Fix fontSizeChange event for Restore Font Size command #7443

Merged
merged 3 commits into from
Apr 8, 2014
Merged

Fix fontSizeChange event for Restore Font Size command #7443

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34dbe00
Fix fontSizeChange event for Restore Font Size command
lkcampbell Apr 8, 2014
479ba7b
Stop event from firing twice. Make code work with em units.
lkcampbell Apr 8, 2014
1547c8b
Only fire event when there is an adjustment in font size
lkcampbell Apr 8, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/view/ViewCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,17 +102,29 @@ define(function (require, exports, module) {
* @param {string=} fontSizeStyle A string with the font size and the size unit
*/
function _setSizeAndRestoreScroll(fontSizeStyle) {
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
scrollPos = editor.getScrollPos(),
line = editor._codeMirror.lineAtHeight(scrollPos.y, "local");
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
oldFontSize = $(".CodeMirror").css("font-size"),
newFontSize = "",
delta = 0,
adjustment = 0,
scrollPos = editor.getScrollPos(),
line = editor._codeMirror.lineAtHeight(scrollPos.y, "local");

_removeDynamicFontSize();
if (fontSizeStyle) {
_addDynamicFontSize(fontSizeStyle);
}
editor.refreshAll();

delta = /em$/.test(oldFontSize) ? 10 : 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think delta for em is 0.1, not 10. Look at the one used in _adjustFontSize function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RaymondLim it's a 10:1 ratio for ems:pixel. The code you refer to is a 1:0.1 ratio which equates to 10:1. If the units are in ems, we need to multiply them by 10 to convert them into pixels. I think keeping everything in pixels is the best bet even if we are using ems because, I think, the calculated font-size always comes back in pixels regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, if I am being completely honest with both of you, I haven't tested this with setting the font size to em units...so, I probably should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomMalbran, if the value of $(".CodeMirror").css("font-size") always comes back in pixels, and I think it does, we don't have to check for ems, correct? Or am I wrong about it always coming back as pixels?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that on chrome it does return the calculated value, but not
sure about other browsers or if the api might change once chrome is able to
return the original value.

Anyway this is just for completition since the rest of the code aaaumes it
can be either value. We probably won't be able to test it. But if we don't
add it here we should assume that we alwaya get px and remove the em bit
and be sure that it will work on other browsers. Ao maybe that is for
another clean up, and I think that it might have been discussed before.
El 08/04/2014 02:09, "Lance Campbell" notifications@github.com escribió:

In src/view/ViewCommandHandlers.js:

@@ -113,6 +117,11 @@ define(function (require, exports, module) {
}
editor.refreshAll();

  •    delta = /em$/.test(oldFontSize) ? 10 : 1;

@TomMalbran https://github.com/TomMalbran, if the value of
$(".CodeMirror").css("font-size") always comes back in pixels, and I
think it does, we don't have to check for ems, correct? Or am I wrong about
it always coming back as pixels?

Reply to this email directly or view it on GitHubhttps://github.com//pull/7443/files#r11376458
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomMalbran, okay, sounds good. Thanks for addressing all my questions.

@RaymondLim, I tested the code setting the font size to em units. It works; however, as @TomMalbran and I were discussing, that branch of the code never gets run. Currently, all font sizes contained in variables oldFontSize and newFontSize are always in terms of pixels. I addressed your question on the math of em ratios as best I could so I think this code review is done. I have nothing to change in the code at this point.

newFontSize = $(".CodeMirror").css("font-size");
adjustment = parseInt((parseFloat(newFontSize) - parseFloat(oldFontSize)) * delta, 10);

if (adjustment) {
$(exports).triggerHandler("fontSizeChange", [adjustment, newFontSize]);
}

// Calculate the new scroll based on the old font sizes and scroll position
var newWidth = editor._codeMirror.defaultCharWidth(),
deltaX = scrollPos.x / oldWidth,
Expand Down Expand Up @@ -153,7 +165,6 @@ define(function (require, exports, module) {
_setSizeAndRestoreScroll(fsStr);
PreferencesManager.setViewState("fontSizeStyle", fsStr);

$(exports).triggerHandler("fontSizeChange", [adjustment, fsStr]);
return true;
}

Expand Down