Skip to content

Fix default action for deleted shares - #19698

Merged
rullzer merged 1 commit into
masterfrom
fix/fix_image_preview_deleted_shares
Mar 10, 2020
Merged

Fix default action for deleted shares#19698
rullzer merged 1 commit into
masterfrom
fix/fix_image_preview_deleted_shares

Conversation

@GretaD

@GretaDGretaD commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

The problem:
When you click the name/thumbnail of a deleted share, you will get a not found message (pic below) which is correct, because the file is deleted and you're not suppose to see a preview. But also you're not suppose to see the error message
notfound

Solution:
Not to be able to click on deleted shares

@GretaD
GretaDforce-pushed the fix/fix_image_preview_deleted_shares branch 2 times, most recently from 45cb7cb to c510a60CompareFebruary 28, 2020 16:04

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

Still pending:

  • Although right now the link can not be clicked there is still a link; probably the a element should be replaced with p or something like that when defaultFileActionsDisabled === true (and in that case maybe the preventDefault() will be no longer needed).
  • Whether the link is replaced with a different element or not, the cursor when hovering on the name and the thumbnail should be the default cursor instead of a pointer cursor, as a pointer cursor conveys that the element being hovered can be interacted with.

@GretaD

GretaD commented Mar 4, 2020

Copy link
Copy Markdown
ContributorAuthor
  • the cursor for the thumbnail should be default too

@GretaD
GretaDforce-pushed the fix/fix_image_preview_deleted_shares branch from 6ac7d74 to dff944bCompareMarch 5, 2020 12:19
@GretaDGretaD added 3. to review Waiting for reviews bug labels Mar 5, 2020
Comment threadapps/files/js/filelist.js Outdated
@GretaD
GretaD marked this pull request as ready for review March 5, 2020 14:43
@GretaDGretaD changed the title WIP - Fix default action for deleted sharesFix default action for deleted sharesMar 5, 2020

@ChristophWurstChristophWurst 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.

Code looks good

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

var renaming = $tr.data('renaming');
if (!renaming) {
if (this._defaultFileActionsDisabled) {
event.preventDefault();

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.

preventDefault() should not be needed now that the a element is replaced by a p element when the default file actions are disabled, but it does no harm either, so 👍

@GretaD
GretaDforce-pushed the fix/fix_image_preview_deleted_shares branch 3 times, most recently from 786ef28 to 89f55ebCompareMarch 9, 2020 16:58
@rullzer
rullzerforce-pushed the fix/fix_image_preview_deleted_shares branch from 89f55eb to e8a3699CompareMarch 9, 2020 20:08
@rullzer

Copy link
Copy Markdown
Member

/compile amend /

Signed-off-by: GretaD <gretadoci@gmail.com>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@npmbuildbot-nextcloud
npmbuildbot-nextcloudBotforce-pushed the fix/fix_image_preview_deleted_shares branch from e8a3699 to b201121CompareMarch 9, 2020 20:16
@gary-kimgary-kim added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 10, 2020
@rullzer
rullzer merged commit 05c6086 into masterMar 10, 2020
@rullzer
rullzer deleted the fix/fix_image_preview_deleted_shares branch March 10, 2020 14:52
@rullzer

Copy link
Copy Markdown
Member

/backport to stable18

@rullzer

Copy link
Copy Markdown
Member

/backport to stable17

@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable17 failed. Please do this backport manually.

@backportbot-nextcloud

Copy link
Copy Markdown

The backport to stable18 failed. Please do this backport manually.

@MorrisJobke

Copy link
Copy Markdown
Member

Caused a regression that deleted group shares cannot be restored anymore: #23240

"href": linkUrl
});
if (this._defaultFileActionsDisabled) {
linkElem = $('<p></p>').attr({

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.

I guess the regression is because the sharing code relies on the <a> tag instead of a <p> tag and then some element appending doesn't work anymore.

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.

Maybe @skjnldsv or @PVince81 can help me here. I can't find anything obvious, but also the file list with their sharing integration is quite a big code base.

Regression can be found in #23240 (comment)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@GretaD@rullzer@MorrisJobke@ChristophWurst@danxuliu@gary-kim@skjnldsv