feature/deseng751: Added ability to use an external link as if it is an internal file. #126
Conversation
jareth-whitney
commented
Jan 10, 2025
jareth-whitney
commented
- Added the ability to add an external link as if it is an internal file. DESENG-751
- Functionality works in all file views, including file list, file details, file add/edit, comment period view, comment period edit.
- All file list functionality works, including open/download, publish, unpublish, edit, and delete.
- All upper-right context menu items are working from file list, file details, etc.
- No multi-edit for external link files yet.
…an internal file.
There was a problem hiding this comment.
Thanks for this! Lots of work involved to make this feature a reality - appreciate your efforts :)
Please have a look at my comments. In particular, could you:
- Remove any console.log calls
- Ensure links open in the same tab unless they're subject to exception from WCAG Guideline G200
- Remove any transformations of URLs that may alter the casing. URLs paths should mostly be treated as case sensitive
src/app/models/externalLink.ts
Outdated
| @@ -0,0 +1,29 @@ | |||
| import * as _ from 'lodash'; | |||
| dateUpdated: Date; | ||
| description: string; | ||
| projectPhase: string; | ||
| checkbox: boolean; |
There was a problem hiding this comment.
Just curious what this property is
| @@ -25,7 +26,8 @@ import { encode } from 'punycode'; | |||
| export class AddDocumentComponent implements OnInit, OnDestroy { | |||
| public terms = new SearchTerms(); | |||
| private ngUnsubscribe: Subject<boolean> = new Subject<boolean>(); | |||
| public documents: Document[] = null; | |||
| public documents = null; | |||
There was a problem hiding this comment.
Just curious why you removed the type for this one. Maybe I can help restore it?
There was a problem hiding this comment.
As discussed, this is to prevent errors that arise from combining types in an array and then referencing properties. It could be handled more strictly with more verbosity, but that would not be in line with the degree of typing that is present in the existing codebase.
| @@ -50,6 +51,7 @@ export class AddEditCommentPeriodComponent implements OnInit, OnDestroy { | |||
| private commentPeriodService: CommentPeriodService, | |||
| private config: ConfigService, | |||
| private surveyService: SurveyService, | |||
| private externalLinkService: LinkService, | |||
There was a problem hiding this comment.
the indentation got a little squirrelly here
There was a problem hiding this comment.
Will replace tab characters with spaces
| .takeUntil(this.ngUnsubscribe) | ||
| .subscribe( | ||
| data => { | ||
| console.log('made it here'); |
There was a problem hiding this comment.
Ah yes I caught that just after I submitted my PR. It was removed.
| * @return {void} | ||
| */ | ||
| public validateLink() { | ||
| const link = this.myForm.value.externalLink.toLowerCase(); |
There was a problem hiding this comment.
URLs have case-sensitive parts. This may break some URLs
| this.tableParams.totalListItems = 0; | ||
| this.documents = []; | ||
| } | ||
| const documents = res.documents?.documents[0] || []; |
There was a problem hiding this comment.
Okay this looks to be just a bug in the diff presentation on Github. Please disregard my previous comments about indentation
| const safeName = item.documentFileName.replace(/ /g, '_'); | ||
| selBox.value = `${this.pathAPI}/document/${item._id}/fetch/${safeName}`; | ||
| } else { | ||
| selBox.value = item.documentFileName; |
There was a problem hiding this comment.
| selBox.value = item.documentFileName; | |
| selBox.value = item.externalLink; |
IMO, it would be more readable to do this instead of assigning documentFileName to externalLink
There was a problem hiding this comment.
Ah I see what you're saying. So in the case of the data tables, I mapped all the data before feeding it into the data table component, so that I gave it uniform values regardless of the source: external link or document. So in this case, I had already mapped the external link to another property because the property externalLink doesn't exist in the format that I feed the table component.
| } | ||
|
|
||
| onDownload() { | ||
| if (this.document.externalLink) { | ||
| window.open(this.document.externalLink, "_blank"); |
There was a problem hiding this comment.
WCAG guidelines state that even external links must open in the same tab whenever possible. One of the exceptions is if you're in the middle of filling out a form. Is that the case here? Otherwise, it should open in the same tab.
There was a problem hiding this comment.
I removed some instances of "_blank". I believe a document open happens in a new tab by default though.
| break; | ||
| case 'delete': | ||
| this.onDeleteDocument(); | ||
| break; | ||
| case 'download': | ||
| this.documentTableData.data.map((item) => { | ||
| if (item.checkbox === true) { | ||
| promises.push(this.api.downloadDocument(this.documents.filter(d => d._id === item._id)[0])); | ||
| if ('external' === item.internalExt) { | ||
| window.open(item.documentFileName, "_blank"); |
There was a problem hiding this comment.
Links should open only in the same tab unless a form is being filled out or other exceptions: https://www.w3.org/WAI/WCAG21/Techniques/general/G200
…odified window.open calls.
| @@ -274,6 +274,7 @@ export class CommentPeriodDetailsTabComponent implements OnInit, OnChanges, OnDe | |||
| * @returns {Promise<void>} | |||
| */ | |||
| public downloadDocument(document) { | |||
| console.log('ran download document code'); | |||
