-
Notifications
You must be signed in to change notification settings - Fork 158
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
[full-ci] Add pdf-viewer app #6654
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/24077/56/1 |
PDF opening acceptance test now fails with
Needs an update, let's quickly discuss this tomorrow! Update:
|
Another update: The PDF viewer app wasn't added in any of the configs yet, so CI wouldn't find it in the contextMenu for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an appropriate reviewer since I collaborated on this one, code LGTM though, waiting for @kulmann's final review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick regarding translations, otherwise awesome! 🚀
name: 'pdf-viewer', | ||
meta: { | ||
auth: false, | ||
title: $gettext('PDF Viewer app'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title: $gettext('PDF Viewer app'), | |
title: $gettext('PDF Viewer'), |
] | ||
|
||
const appInfo = { | ||
name: 'PDF viewer', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: 'PDF viewer', | |
name: $gettext('PDF Viewer'), |
SonarCloud Quality Gate failed. |
Supersedes #6575
Description
This adds a pdf viewer app which uses the native pdf support of the browser.
This cleans up the code of the original PR and makes it to good to merge imho (for a first iteration at least) when the CI is green.
We should discuss
newTab
and should add a way to navigate back (cc @tbsbdr @pascalwengerter @kulmann ) in followup tickets/prs.Related Issue
Motivation and Context
among other reasons: blob urls are not bookmarkable
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: