-
Notifications
You must be signed in to change notification settings - Fork 785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667
base: master
Are you sure you want to change the base?
Conversation
Also solves: |
Well, you got me, I'm no PHP dev, not even close, I guess the |
Yes, that's sometimes annoying. I guess you know you should use the editorconfig plugin or so for your editor in case it is not built-in. We use that in our repo, so it should notice and use that. That should actually fix that. Anyway as for styleci: It has a "diff download" feature, which you can then apply via the |
Cool, this should be an easy fix. |
*/ | ||
me.humanizeBytes = function(bytes, decimals = 2) { | ||
const sizes = [ | ||
I18n._('Bytes'), I18n._('KB'), I18n._('MB'), I18n._('GB'), |
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.
Purely a suggestion, as this is fully functional in the current state: Keep the untranslated strings in the sizes array, then translate only the needed one in line 383 (me.sprintf('0 %s', I18n._(sizes[0]));
). This makes the array more readable and avoids unnecessary translations.
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.
Thanks a bunch, especially for also updating the unit tests accordingly.
]; | ||
const signifierByte = I18n._('Byte'); | ||
if (bytes === 0) return me.sprintf('0 %s', sizes[0]); | ||
if (bytes === 1) return me.sprintf('1 %s', signifierByte); |
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.
If I understand the intent behind these two lines correctly, you want to translate "Byte" differently, if it applies to 0 instead of 1. I would recommend to let the translation systems plural form support handle this, as in different languages different rules apply for the use of a plural form for 0 or other numbers - in English and German we do need to use the plural for 0 of something, but for example in French or Chinese this not the case:
Lines 781 to 784 in 33bcce5
case 'fr': | |
case 'oc': | |
case 'zh': | |
return n > 1 ? 1 : 0; |
So my suggestion would be to instead have a line like (the translate method supports sprintf like syntax as well):
if (bytes < 1024) return I18n._('%d Byte', bytes);
me.humanizeBytes = function(bytes, decimals = 2) { | ||
const sizes = [ | ||
I18n._('Bytes'), I18n._('KB'), I18n._('MB'), I18n._('GB'), | ||
I18n._('TB'), I18n._('PB'), I18n._('EB'), I18n._('ZB'), I18n._('YB') |
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.
Please use the power of 1024 based binary prefixes to avoid any confusion with the power of 1000 based SI unit prefixes. The former ones already got translated for those languages that do use a different unit letter - for example the "o" for "octet" in French:
Lines 123 to 131 in 33bcce5
"B": "o", | |
"KiB": "Kio", | |
"MiB": "Mio", | |
"GiB": "Gio", | |
"TiB": "Tio", | |
"PiB": "Pio", | |
"EiB": "Eio", | |
"ZiB": "Zio", | |
"YiB": "Yio", |
@@ -2763,8 2787,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { | |||
// extract mediaType | |||
const mediaType = attachmentData.substring(5, mediaTypeEnd); | |||
// extract data and convert to binary | |||
const rawData = attachmentData.substring(base64Start); | |||
const decodedData = rawData.length > 0 ? atob(rawData) : ''; |
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.
This line probably got accidentally reverted. I had added the length check recently to address the bug reported in #663. I am fine to remove the rawData
assignment, though, as keeping this in a variable only supports readability, but it isn't reused further on.
|
||
// pevent '#' from appearing in the URL | ||
if (typeof event !== 'undefined') { | ||
event.preventDefault(); |
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.
Very, very minor nitpick - please indent lines 3783 and 3784, now that these are part of an if-block. :-)
/* file name may also be displayed in alert */ | ||
.alert { | ||
word-wrap: break-word; | ||
} |
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.
AFAIK a line ending is missing here. It's not important, but do you use editorconfig as per our contrib guide?
IIRC it should handle that newline at the end, too.
Next time maybe try to install/use an editorconfig plugin or so for your editor in case it is not built-in. We use that in our repo, so it should notice and use that.
@@ -408,7 437,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { | |||
DOMPurify.sanitize( | |||
element.html().replace( | |||
/(((https?|ftp):\/\/[\w?!=&.\/-;#@~% *-] (?![\w\s?!&.\/;#~%"=-]>))|((magnet):[\w?=&.\/-;#@~% *-] ))/ig, | |||
'<a href="$1" rel="nofollow noopener noreferrer">$1</a>' | |||
'<a href="$1" target="_blank" rel="nofollow noopener noreferrer">$1</a>' |
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.
Interesting we did not use this here… Should have been used, since we actually want that.
I've searched some issues and this should be fixed since #61 already and #451.
As we use noopener
here though, that should be fine.
(But please check DomPurify does not remove that or so, so it is properly sanitized.)
@@ -2802,7 2825,18 @@ jQuery.PrivateBin = (function($, RawDeflate) { | |||
me.showAttachment = function() | |||
{ | |||
$attachment.removeClass('hidden'); | |||
me.showAttachmentPreview(); | |||
|
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.
empty line
$panel.append($preview); | ||
} | ||
$targetElement.append($panel); | ||
// consider preview available if name and size can be displyed |
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.
// consider preview available if name and size can be displyed | |
// consider preview available if name and size can be displayed |
A little embarrassing to have you find these🙈 |
Hehe, it's all right, that's what reviews are for. Even though these typos in comments are not a problem at all… 🙃 |
This PR fixes
Part of:
#625
Test site: https://privatebin-testing.herokuapp.com/
close #603
Changes
Display file name and size even when not previewable:
Display file name and size along with the preview:
ToDo