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

[full-ci] Add pdf-viewer app #6654

Merged
merged 11 commits into from
Mar 24, 2022
Merged

[full-ci] Add pdf-viewer app #6654

merged 11 commits into from
Mar 24, 2022

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Mar 23, 2022

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

  • Fixes <issue_link>

Motivation and Context

among other reasons: blob urls are not bookmarkable

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Mar 23, 2022

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.

@ownclouders
Copy link
Contributor

ownclouders commented Mar 23, 2022

Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/24077/56/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIFilesActionMenu-fileFolderActionMenu_feature-L14.png

webUIFilesActionMenu-fileFolderActionMenu_feature-L14.png

@pascalwengerter
Copy link
Contributor

pascalwengerter commented Mar 23, 2022

Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/24077/56/1
💥 The acceptance tests failed on retry. Please find the screenshots inside ...

PDF opening acceptance test now fails with

   ✔ Then the app-sidebar for file "lorem.pdf" should be visible on the webUI # stepDefinitions/filesContext.js:1025

   ✖ And only the following items with default items should be visible in the actions menu on the webUI # stepDefinitions/filesContext.js:1365

Needs an update, let's quickly discuss this tomorrow!

Update:
There's two failures in CI,

@pascalwengerter
Copy link
Contributor

Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/24077/56/1
💥 The acceptance tests failed on retry. Please find the screenshots inside ...

PDF opening acceptance test now fails with

   ✔ Then the app-sidebar for file "lorem.pdf" should be visible on the webUI # stepDefinitions/filesContext.js:1025

   ✖ And only the following items with default items should be visible in the actions menu on the webUI # stepDefinitions/filesContext.js:1365

Needs an update, let's quickly discuss this tomorrow!

Update: There's two failures in CI,

* `webUIFilesActionMenu/fileFolderActionMenu.feature:14` => see text above

* `webUIPrivateLinks/accessingPrivateLinks.feature:9` => see [oc10IntegrationApp1 private link acceptance tests flaky #6607](https://github.com/owncloud/web/issues/6607)

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 .pdf files. Should be fine now, I've added it as enabled by default to all configs I could find

@pascalwengerter pascalwengerter mentioned this pull request Mar 23, 2022
Copy link
Contributor

@pascalwengerter pascalwengerter left a 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

Copy link
Contributor

@kulmann kulmann left a 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'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: $gettext('PDF Viewer app'),
title: $gettext('PDF Viewer'),

]

const appInfo = {
name: 'PDF viewer',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: 'PDF viewer',
name: $gettext('PDF Viewer'),

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit 81cd15e into master Mar 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the pdf_viewer branch March 24, 2022 16:15
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.

5 participants