625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667

Haocen · 2020-07-05T16:24:47Z

This PR fixes
Part of:
#625

Test site: https://privatebin-testing.herokuapp.com/

close #603

Changes

Make urls2links match expected test outcome
Avoid iOS auto zoom into textarea
Completely rework attatchment preview related logic, allowing showing the attachment preview in the preview tab right after it is added, besides the file name and file size. The same change applies to paste reading view.

Display file name and size even when not previewable:

Display file name and size along with the preview:

ToDo

[ ]
[ ]
[ ]

Haocen · 2020-07-05T16:27:45Z

Also solves:
#603

Haocen · 2020-07-05T16:31:23Z

Well, you got me, I'm no PHP dev, not even close, I guess the styleci is complaining mixed use of tab and space?
Not sure how to fix it, though...

rugk · 2020-07-08T12:11:13Z

, I guess the styleci is complaining mixed use of tab and space?

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 diff command.

Haocen · 2020-07-08T12:26:45Z

Cool, this should be an easy fix.

elrido · 2020-07-08T18:20:59Z

js/privatebin.js

+         */
+        me.humanizeBytes = function(bytes, decimals = 2) {
+            const sizes = [
+                I18n._('Bytes'), I18n._('KB'), I18n._('MB'), I18n._('GB'),


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.

elrido

Thanks a bunch, especially for also updating the unit tests accordingly.

elrido · 2020-07-08T18:41:28Z

js/privatebin.js

+            ];
+            const signifierByte = I18n._('Byte');
+            if (bytes === 0) return me.sprintf('0 %s', sizes[0]);
+            if (bytes === 1) return me.sprintf('1 %s', signifierByte);


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:

PrivateBin/js/privatebin.js

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);

elrido · 2020-07-08T18:46:09Z

js/privatebin.js

+        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')


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:

PrivateBin/i18n/fr.json

Lines 123 to 131 in 33bcce5

"B": "o",

"KiB": "Kio",

"MiB": "Mio",

"GiB": "Gio",

"TiB": "Tio",

"PiB": "Pio",

"EiB": "Eio",

"ZiB": "Zio",

"YiB": "Yio",

elrido · 2020-07-08T18:53:08Z

js/privatebin.js

@@ -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) : '';


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.

elrido · 2020-07-08T18:56:35Z

js/privatebin.js


            // pevent '#' from appearing in the URL
+            if (typeof event !== 'undefined') {
            event.preventDefault();


Very, very minor nitpick - please indent lines 3783 and 3784, now that these are part of an if-block. :-)

rugk · 2020-07-09T11:55:36Z

css/bootstrap/privatebin.css

+/* file name may also be displayed in alert */
+.alert {
+	word-wrap: break-word;
+}


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.

rugk · 2020-07-09T12:02:52Z

js/privatebin.js

@@ -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>'


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.)

rugk · 2020-07-09T12:09:15Z

js/privatebin.js

@@ -2802,7 +2825,18 @@ jQuery.PrivateBin = (function($, RawDeflate) {
        me.showAttachment = function()
        {
            $attachment.removeClass('hidden');
+            me.showAttachmentPreview();
+


Suggested change

empty line

rugk · 2020-07-09T12:20:06Z

js/privatebin.js

+                        $panel.append($preview);
+                    }
+                    $targetElement.append($panel);
+                    // consider preview available if name and size can be displyed


Suggested change

// consider preview available if name and size can be displyed

// consider preview available if name and size can be displayed

Haocen · 2020-07-09T14:08:24Z

A little embarrassing to have you find these🙈
I'll make sure my weekend coffee is as strong as my weekday ones so I no long make mistakes like these...

rugk · 2020-07-09T17:18:48Z

Hehe, it's all right, that's what reviews are for. Even though these typos in comments are not a problem at all… 🙃

Haocen added 5 commits July 4, 2020 23:57

Make urls2links match expected test outcome

2cdf201

Avoid iOS auto zoom into textarea

1f00936

Completely rework attatchment preview related logic

4e70601

Update SRI

fab7a34

Fix tests

6a6a181

Haocen changed the title ~~625 enhancements~~ 625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea Jul 5, 2020

rugk requested review from elrido and rugk July 8, 2020 12:11

elrido reviewed Jul 8, 2020

View reviewed changes

elrido requested changes Jul 8, 2020

View reviewed changes

rugk requested changes Jul 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667

625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667

Haocen commented Jul 5, 2020 •

edited

Haocen commented Jul 5, 2020

Haocen commented Jul 5, 2020

rugk commented Jul 8, 2020

Haocen commented Jul 8, 2020

elrido Jul 8, 2020

elrido left a comment

elrido Jul 8, 2020

elrido Jul 8, 2020

elrido Jul 8, 2020

elrido Jul 8, 2020

rugk Jul 9, 2020

rugk Jul 9, 2020

rugk Jul 9, 2020

rugk Jul 9, 2020

Haocen commented Jul 9, 2020

rugk commented Jul 9, 2020 •

edited

	"B": "o",
	"KiB": "Kio",
	"MiB": "Mio",
	"GiB": "Gio",
	"TiB": "Tio",
	"PiB": "Pio",
	"EiB": "Eio",
	"ZiB": "Zio",
	"YiB": "Yio",

	// consider preview available if name and size can be displyed
	// consider preview available if name and size can be displayed

625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667

Are you sure you want to change the base?

625 enhancements - Completely rework attatchment preview related logic, Avoid iOS auto zoom into textarea #667

Conversation

Haocen commented Jul 5, 2020 • edited

Changes

ToDo

Haocen commented Jul 5, 2020

Haocen commented Jul 5, 2020

rugk commented Jul 8, 2020

Haocen commented Jul 8, 2020

Choose a reason for hiding this comment

elrido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haocen commented Jul 9, 2020

rugk commented Jul 9, 2020 • edited

Haocen commented Jul 5, 2020 •

edited

rugk commented Jul 9, 2020 •

edited