Skip to content

Fix user selectable text for public links for text files - #9206

Merged
rullzer merged 1 commit into
masterfrom
fix-select-text
Apr 17, 2018
Merged

Fix user selectable text for public links for text files#9206
rullzer merged 1 commit into
masterfrom
fix-select-text

Conversation

@MorrisJobke

Copy link
Copy Markdown
Member

Makes public link share text files selectable.

Was added in owncloud/core#15652

As discussed with @jancborchardt - cc @nextcloud/designers

Found while reviewing https://github.com/nextcloud/files_texteditor/pull/71/files

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@codecov

codecovBot commented Apr 16, 2018

Copy link
Copy Markdown

Codecov Report

Merging #9206 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #9206 +/- ##
===========================================
+ Coverage 51.9% 51.9% +<.01% 
Complexity 25354 25354 ===========================================
Files 1606 1606 Lines 95301 95301 Branches 1394 1394 ===========================================
+ Hits 49465 49466 +1 + Misses 45836 45835 -1
Impacted FilesCoverage ΔComplexity Δ
apps/files_trashbin/lib/Trashbin.php72.7% <0%> (+0.24%)136% <0%> (ø)⬇️

@oparoz

Copy link
Copy Markdown
Member

I don't think it's the right solution since we only get the first few characters, but if it matches what's done elsewhere then it's probably OK.

@danxuliudanxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

Note that this change affects too the Text editor app.

For reference, as mentioned in nextcloud/files_texteditor#71 (comment) the height of the text preview is cropped (down to 200px) to ensure that the Download button is visible. When the preview is cropped and the text is selected part of the hidden text could end being selected too, which could be surprising for the user. However, as since Nextcloud 13 the Download button is shown in the header too and thus it is always visible the cropping could be removed (from both the default preview and the Text editor app preview), which would make this issue go away ;-)

@danxuliu

Copy link
Copy Markdown
Member

@oparoz

I don't think it's the right solution since we only get the first few characters, but if it matches what's done elsewhere then it's probably OK.

Making the text selectable was approved by @jancborchardt, so it should be OK ;-) (although he was referring to the Text editor app, which loads a lot more text than the default preview, but I suppose that it is OK in this case too).

@rullzer

Copy link
Copy Markdown
Member

@oparoz well it is a preview sure. But still having real text there and not being selectable is a bti weird ;)

@rullzer
rullzer merged commit cf27310 into masterApr 17, 2018
@rullzer
rullzer deleted the fix-select-text branch April 17, 2018 08:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MorrisJobke@oparoz@danxuliu@rullzer