[ENG-11452] My Projects Page: Download Button#1018
Conversation
|
@nsemets Looks like there are some conflicts now. |
…ts' into feat/ENG-11452
|
@brianjgeiger Fixed conflicts. |
| readonly showDownloadColumn = input<boolean>(false); | ||
| readonly emptyMessageKey = input<string>('common.search.noResultsFound'); | ||
| readonly downloadCellTemplate = input<TemplateRef<{ $implicit: MyResourcesItem }>>(); | ||
|
|
||
| readonly pageChange = output<TablePageEvent>(); | ||
| readonly sort = output<SortEvent>(); | ||
| readonly itemClick = output<MyResourcesItem>(); | ||
|
|
||
| readonly skeletonData = Array.from({ length: 10 }, () => ({}) as MyResourcesItem); | ||
|
|
||
| readonly columnCount = computed(() => (this.showDownloadColumn() ? 4 : 3)); |
There was a problem hiding this comment.
showDownloadColumn and downloadCellTemplate are two inputs that must always be set together if one is passed without the other you get a broken column silently. Since the column only makes sense when a template exists, just drive the @if and columnCount from downloadCellTemplate() directly and drop the boolean input entirely.
| readonly showDownloadColumn = input<boolean>(false); | ||
| readonly emptyMessageKey = input<string>('common.search.noResultsFound'); | ||
| readonly downloadCellTemplate = input<TemplateRef<{ $implicit: MyResourcesItem }>>(); | ||
|
|
||
| readonly pageChange = output<TablePageEvent>(); | ||
| readonly sort = output<SortEvent>(); | ||
| readonly itemClick = output<MyResourcesItem>(); | ||
|
|
||
| readonly skeletonData = Array.from({ length: 10 }, () => ({}) as MyResourcesItem); | ||
|
|
||
| readonly columnCount = computed(() => (this.showDownloadColumn() ? 4 : 3)); |
There was a problem hiding this comment.
The 3 in computed(() => (this.showDownloadColumn() ? 4 : 3)) is a silent assumption about the number of fixed columns. If a column is added later this breaks without warning. Extract it as a named constant: private static readonly BASE_COLUMN_COUNT = 3.
|
|
||
| const addonName = getConfiguredStorageAddonDisplayName(configuredAddons, folder.provider, ''); | ||
|
|
||
| if (!addonName || !folder.links.download) { |
There was a problem hiding this comment.
OSF Storage correctly hides the option when totalCount === 0, but connected add-ons show regardless of whether they have any files. The requirements say "avoid showing options that would result in empty downloads", an empty add-on zip would violate that. N+1 calls to check each add-on's file count may not be ideal, but worth aligning with product on whether an empty zip download is acceptable before merging.
There was a problem hiding this comment.
No, sorry, the requirement is not ideally worded. Only osfstorage should be gated on files being there; the other storage addons, if they are connected, will show the download links, without us checking for files. The assumption being that, if someone has connected an addon, they have probably added a file or more to it.
| </th> | ||
|
|
||
| @if (showDownloadColumn()) { | ||
| <th aria-hidden="true"></th> |
There was a problem hiding this comment.
I believe <th aria-hidden="true"> hides the column entirely from screen readers. A visually hidden label is better: <th><span class="sr-only">{{ 'common.labels.download' | translate }}</span></th> to keeps it discoverable when navigating the table by column.
There was a problem hiding this comment.
I think it is better to use [attr.aria-label] here.
3 participants
Summary of Changes
Screenshot(s)