Skip to content

[FIX] Attachment download from title fixed #20585

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.

Sign up for GitHub

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

Jump to bottom
Merged
ggazzo merged 6 commits into from
Feb 9, 2021
Merged

[FIX] Attachment download from title fixed #20585

ggazzo merged 6 commits into from
Feb 9, 2021

Conversation

@yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Feb 3, 2021 •
edited
Loading

  • I have read the Contributing Guide
  • I have signed the CLA - https://cla-assistant.io/RocketChat/Rocket.Chat
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Proposed changes (including videos or screenshots)

Added target = '_self' to attachment link, this seems to fix the problem, without this attribute, error page is displayed.

Issue(s)

fixes #20583 #20559

Steps to test or reproduce

given on issue

Further comments

fix-attachment-download-after1

@yash-rajpal
Copy link
Member Author

yash-rajpal commented Feb 3, 2021

@ggazzo Please Review. :)

@yash-rajpal
Copy link
Member Author

yash-rajpal commented Feb 3, 2021 •
edited
Loading

Changed target to self, seems to solve the issue, without opening new tab.

@ggazzo ggazzo requested a review from a team February 3, 2021 15:47
ggazzo
ggazzo previously approved these changes Feb 3, 2021
View reviewed changes
@ggazzo ggazzo added this to the 3.11.1 milestone Feb 3, 2021
@ggazzo ggazzo requested a review from MartinSchoeler February 4, 2021 02:30
MartinSchoeler
MartinSchoeler previously requested changes Feb 4, 2021
View reviewed changes
Copy link
Member

@MartinSchoeler MartinSchoeler left a comment

Choose a reason for hiding this comment

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

Hey, could you check this Pull Request on our Electron (Desktop) app? It seems to be opening a browser window when you click on the download icon. Thanks!

@MartinSchoeler MartinSchoeler dismissed ggazzo’s stale review February 4, 2021 15:20

there is still an issue with the PR

@ggazzo
Copy link
Member

ggazzo commented Feb 4, 2021

try change self to blank

@yash-rajpal
Copy link
Member Author

yash-rajpal commented Feb 4, 2021

Sir, I checked both download icon and title link work fine and open in electron app only. In earlier commits, I had target as _blank but I figured keeping it as blank would open new browser window and hence I changed it to self due to this reason.
Self doesn't opens a new window. I am also attaching a gif as proof.
download-attachment-2
Thanks @MartinSchoeler , Please review. :)

@yash-rajpal
Copy link
Member Author

yash-rajpal commented Feb 4, 2021

Actually in my testing both work fine on desktop app, so I am changing target to _blank only. Below is a gif with target set to blank.
attachment-download-3

@yash-rajpal yash-rajpal requested a review from ggazzo February 4, 2021 16:34
@tiagoevanp tiagoevanp self-requested a review February 4, 2021 19:58
@yash-rajpal yash-rajpal mentioned this pull request Feb 5, 2021
Closed
@ggazzo ggazzo merged commit b42acf2 into RocketChat:develop Feb 9, 2021
sampaiodiego pushed a commit that referenced this pull request Feb 10, 2021
@yash-rajpal @tiagoevanp @sampaiodiego
[FIX] Attachment download from title fixed (#20585)
c53a499 
Co-authored-by: Tiago Evangelista Pinto <[email protected]>
@sampaiodiego sampaiodiego mentioned this pull request Feb 10, 2021
Merged
sampaiodiego pushed a commit that referenced this pull request Feb 10, 2021
@yash-rajpal @tiagoevanp @sampaiodiego
[FIX] Attachment download from title fixed (#20585)
e4e335d 
Co-authored-by: Tiago Evangelista Pinto <[email protected]>
vanhoang1107 pushed a commit to vanhoang1107/Rocket.Chat that referenced this pull request Feb 19, 2021
Merge remote-tracking branch 'rocketchat/master'
cd137d6 
* rocketchat/master:
  Bump version to 3.11.1
  [FIX] Livechat bridge permission checkers (RocketChat#20653)
  Fix room not being assigned to bot agent first. (RocketChat#20662)
  [FIX] Attachment download from title fixed (RocketChat#20585)
  [FIX] Gif images aspect ratio on preview (RocketChat#20654)
  [FIX] Update NPS banner when changing score (RocketChat#20611)
@sampaiodiego sampaiodiego mentioned this pull request Feb 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MartinSchoeler MartinSchoeler Awaiting requested review from MartinSchoeler

@ggazzo ggazzo Awaiting requested review from ggazzo

1 more reviewer

@tiagoevanp tiagoevanp tiagoevanp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.11.1

Development

Successfully merging this pull request may close these issues.

Cannot open attachments with accents

4 participants

@yash-rajpal @ggazzo @tiagoevanp @MartinSchoeler