-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
@@ -450,7 +450,7 @@ define(function (require, exports, module) { | |||
$previewContainer.find(".image-preview > img").on("load", function () { | |||
$previewContent | |||
.append("<div class='img-size'>" + | |||
this.naturalWidth + " x " + this.naturalHeight + " " + Strings.UNIT_PIXELS + | |||
this.naturalWidth + " × " + this.naturalHeight + " " + Strings.UNIT_PIXELS + |
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.
Should be ×
instead of raw high-ascii character.
Done with review. |
@RaymondLim fixed. |
@@ -98,7 +98,7 @@ define(function (require, exports, module) { | |||
$("#img-preview").on("load", function () { | |||
// add dimensions and size | |||
_naturalWidth = this.naturalWidth; | |||
var dimensionString = _naturalWidth + " x " + this.naturalHeight + " " + Strings.UNIT_PIXELS; | |||
var dimensionString = _naturalWidth + " × " + this.naturalHeight + " " + Strings.UNIT_PIXELS; |
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.
Now we're using html. So we also need to change .text
on line 106 to .html
.
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.
Do we still need to change 106 even though we're using html() on 110?
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.
Yes, we do. Since dimensionString is no longer plain text and line 110 is for the case where stat call does not return any error. In the rare case that the stat call returns an error (ie, we can't get the image size), then we still want to show dimensionString as html.
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.
Ok fixed.
Merging. |
See commit messages.