Conversation
bram-maegerman
force-pushed
the
pdf-viewer-10.0.0-next
branch
from
e77e2a9 to
b4582ff
Compare
There was a problem hiding this comment.
I have checked the code and integration part seems ok. I have narrowed a few cases in PR comments.
Checklist:
- Check inherit of metadata
- Check library health, license
- Check if viewer works
- including multi-page pdf to test route changes
- Check logic when viewer should be visible
- Accessibility
- Lint check
- Review design of feature
- Tests
Feature feature logic:
In the config we have:
# Whether to enable the PDF viewer for Bitstreams (i.e. Bitstreams whose MIME type starts with 'application/pdf').
pdfViewer:
enabled: trueI understand this feature requires to be enabled to be functional. When we follow this logic there are rough edges to handle:
- If document has
dspace.pdfviewer.enabledset to true, it ignores feature flag - When we do not set enabled to false it will fallback to true thus feature will be enabled
I have marked specific parts of code responsible for it. Please tell if I think wrongly here.
isFullscreen store:
My question is whether this state is tracked only for debugging purposes. This is ok for me to stay but I wonder whether there was any other rationale about it. I assume this is just to check a transition between DSO page <=> pdf viewer.
Priority hierarchy:
This is just for documentation purposes.
Given structure Community > Collection > Item > Bitstream
- Children will inherit metadata from parent
- The last hierarchical element tells whenever PDF Viewer will be enabled or not
- From parent to grandchildren the value is passed as default value but in the end bitstream is aggregate responsible to tell whenever to display it
This behaviour in my opinion is ok.
Accessibility:
Collection page:
Community page:
There is lack of margin between back nad save button. There is global class space-children-mr to handle it.
I have checked item, bitstream page and there is everything ok.
Tests:
I think I would research places where we can add tests. The good start would be:
- PdfViewerService
- PdfViewerEnableComponent
- EditBitstreamPageComponent
src/config/app-config.interface.ts
Outdated
| matomo?: MatomoConfig; | ||
| geospatialMapViewer: GeospatialMapConfig; | ||
| accessibility: AccessibilitySettingsConfig; | ||
| pdfViewer?: { enabled: boolean }; |
There was a problem hiding this comment.
I would move it to the interface like any other feature does it since it is common pattern in the application.
There was a problem hiding this comment.
Look at default-app-config.ts and specify default enabled value like you did for environment.test.ts. This will make code more ergonomic since you do not need to chain ?. everywhere.
| }), | ||
| filter((dso: DSpaceObject) => hasValue(dso)), | ||
| toArray(), | ||
| map((dsos: DSpaceObject[]) => { |
There was a problem hiding this comment.
I think the logic should be conjunctive:
isNotEmpty(dsosWithViewerInfo) && !!environment.pdfViewer?.enabledOtherwise we allow to run PDF Viewer even when feature is disabled by configuration. Please tell me if I think wrongly here.
There was a problem hiding this comment.
Current logic tells that if key is not present then it should fallback as enabled. This connects to former comment whenever it should not be set to false as default.
environment.pdfViewer?.enabled ?? true;| /** | ||
| * Service to check permissions for PDF viewer | ||
| */ | ||
| export class PdfViewerService { |
There was a problem hiding this comment.
Please use naming conventions for observables e.g. isViewerEnabled$. The dollar suffix sign tells it is observable.
| } | ||
| <ng-container *ngTemplateOutlet="content"></ng-container> | ||
| <span> | ||
| <a [routerLink]="pdfViewerPath" [ngClass]="cssClasses" |
There was a problem hiding this comment.
I think there should be set role to link for a11y improvements for anchor tag.
| title="{{'pdf-viewer.link.view' | translate}}"> | ||
| <i class="fa fa-eye ms-2" ngbTooltip="{{'pdf-viewer.link.view' | translate}}" | ||
| container="body"></i></a> | ||
| <a [routerLink]="(bitstreamPath$| async)?.routerLink" [queryParams]="(bitstreamPath$| async)?.queryParams" |
There was a problem hiding this comment.
I think there should be set role to link for a11y improvements for anchor tag.
tdonohue
moved this from 🙋 Needs Reviewers Assigned
to 👀 Under Review
in DSpace 10.0 Release
|
We should assess the stability of the ngx-extended-pdf-viewer project. The project appears to be maintained by a single main developer and contains almost 20,000 lines of TypeScript code under |
bram-maegerman
force-pushed
the
pdf-viewer-10.0.0-next
branch
from
9d0d0ef to
b59f0c0
Compare
|
Thanks for the feedback on this @pzlakowski! I Confirmed that I added tests for:
Please let me know if there is any other feedback! |
4 participants
References
Description
This PR adds a PDF viewer to DSpace
When enabled, users can view PDFs directly in the DSpace UI instead of downloading them. The viewer is based on ngx-extended-pdf-viewer.
Administrators can control PDF toggle the use of the viewer at community, collection, item, and bitstream levels. A new “PDF viewer” tab is added to the edit pages for communities, collections, and items, and a toggle is added to the edit bitstream page.
The file download link is updated so that when the PDF viewer is allowed, users see both a “View” (eye) and “Download” (file-download) icon instead of only a download link.
Instructions for Reviewers
To test the PDF viewer:
List of changes in this PR:
ngx-extended-pdf-viewer.Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.