Skip to content
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

Default to target=_blank for report comment links #601

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Oct 6, 2020

cc @Jag96

Quick fix here. Couldn't think of any reason why we'd need to open a report comment link in app for now so providing defaults for now to prevent people from getting stuck viewing files in the desktop app. Discussed with @Jag96 and the only reason we could think of is that file downloads will bypass the default "Save as..." prompt that shows in Electron. But there's no clear reason why it's better to do that than just opening these links in Chrome for now.

This logic might need to change if we have report comments that are intended to deep link to various places in the app. But that seems like a future problem.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/142225

Tests

  1. Send yourself a .zip attachment and a .txt attachment
  2. Attempt to open both in web and desktop platforms
  3. Verify they always open in a new external browser window

Screenshots

@marcaaron marcaaron self-assigned this Oct 6, 2020
@marcaaron marcaaron requested a review from Jag96 October 6, 2020 19:03
@tgolen tgolen merged commit 1589801 into master Oct 6, 2020
@tgolen tgolen deleted the marcaaron-useTargetBlankForCommentLinks branch October 6, 2020 21:31
@MelvinBot MelvinBot mentioned this pull request Jan 29, 2024
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants