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.

Sign up for GitHub

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

Revamp content file mgt #4872

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open

Revamp content file mgt #4872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9048c3
Implements a meatballs menu to replace, download and remove a partula…
akolson Jan 15, 2025
bbebc7d
Adds tests to check menu options click behaviour
akolson Jan 16, 2025
b452431
implements accessibility requirements
akolson Jan 29, 2025
3bd10ed
fixes cut off focus outline on links
akolson Jan 29, 2025
5d48c6f
Update file name font size
akolson Jan 29, 2025
0a2db6f
Update file name font size
akolson Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,4 +680,8 @@
margin-top: -4px !important;
}

/deep/ .v-dialog__content {
Copy link
Member Author

Choose a reason for hiding this comment

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

@rtibbles noting here that linting doesn't seem to recorgnize ::v-deep (Unexpected unknown pseudo-element selector) So reverted to /deep/ for now

z-index: 5 !important;
}

</style>
55 changes: 28 additions & 27 deletions contentcuration/contentcuration/frontend/channelEdit/views/files/FileUpload.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,32 @@
</VCardText>
</VCard>
<VLayout v-else row wrap>
<VFlex sm12 md6 lg5 xl4>
<VFlex sm12 md12 lg12 xl12>
<p>
<ContentNodeIcon :kind="node.kind" includeText />
</p>
</VFlex>
<VFlex sm12 md6 lg7 xl7 class="pr-4">
<h3>
{{ $tr('filesHeader') }}
</h3>
<VList threeLine>
<FileUploadItem
v-for="item in primaryFileMapping"
:key="item.preset.id"
:file="item.file"
:preset="item.preset"
:allowFileRemove="allowFileRemove"
:uploadCompleteHandler="handleUploadComplete"
@selected="selected = item.file.id"
@remove="handleRemoveFile"
/>
</VList>
</VFlex>
<VFlex sm12 md6 lg5 xl5>
<h3 v-if="selectedFilename" class="mb-3">
{{ selectedFilename }}
</h3>
<div class="preview-wrapper">
<VCard v-if="!primaryFileCount" flat class="mb-2 message-card">
<VLayout align-center justify-center fill-height>
Expand All @@ -39,31 +61,6 @@
/>
</div>
</VFlex>
<VFlex sm12 md6 lg7 xl8>
<VContainer fluid>
<VLayout alignStart>
<VRadioGroup
v-model="selected"
hide-details
:label="$tr('filesHeader')"
class="subheading"
>
<VList threeLine>
<FileUploadItem
v-for="item in primaryFileMapping"
:key="item.preset.id"
:file="item.file"
:preset="item.preset"
:allowFileRemove="allowFileRemove"
:uploadCompleteHandler="handleUploadComplete"
@selected="selected = item.file.id"
@remove="handleRemoveFile"
/>
</VList>
</VRadioGroup>
</VLayout>
</VContainer>
</VFlex>
</VLayout>
</div>

Expand Down Expand Up @@ -134,6 +131,10 @@
'order'
);
},
selectedFilename() {
const file = this.files.find(f => f.id === this.selected);
return file ? file.original_filename : '';
Copy link
Member

Choose a reason for hiding this comment

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

Any opinion of not using use nullish coalescing here?

},
},
watch: {
'files.length'(newCount, oldCount) {
Expand Down Expand Up @@ -171,7 +172,7 @@
},
},
$trs: {
filesHeader: 'Preview files',
filesHeader: 'Files',
fileError: 'Unsupported file type',
noFileText: 'Missing files',
},
Expand Down
118 changes: 74 additions & 44 deletions contentcuration/contentcuration/frontend/channelEdit/views/files/FileUploadItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,19 @@
<VListTile
data-test="list-item"
v-bind="$attrs"
tabindex="0"
@click.stop="file ? $emit('selected') : openFileDialog()"
>
<VListTileAction>
<VRadio
v-if="file"
:key="file.id"
:value="file.id"
color="primary"
data-test="radio"
/>
</VListTileAction>
<VListTileContent>
<VListTileSubTitle>{{ translateConstant(preset.id) }}</VListTileSubTitle>
<VListTileTitle>
<ActionLink
<VListTileTitle class="file-display">
<span
v-if="fileDisplay"
class="notranslate"
:text="formattedFileDisplay"
data-test="file-link"
@click="openFileDialog"
/>
data-test="file-name"
>
{{ formattedFileDisplay }}
</span>
<ActionLink
v-else
data-test="upload-link"
Expand All @@ -53,15 +45,20 @@
</VListTileContent>
<VSpacer />
<VListTileAction v-if="fileDisplay">
<div v-if="allowFileRemove" class="remove-icon">
<IconButton
icon="clear"
color="grey"
:text="$tr('removeFileButton')"
data-test="remove"
@click="$emit('remove', file)"
/>
</div>
<KIconButton
size="small"
icon="optionsHorizontal"
appearance="flat-button"
data-test="show-file-options"
>
<template #menu>
<KDropdownMenu
:options="previewFilesOptions"
data-test="file-options"
Copy link
Member

Choose a reason for hiding this comment

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

You are passing openFileDialog as an argument to every option's onClick handler. But in thepreviewFilesOptions, only the 'REPLACE_FILE' option actually expects that argument? The download and remove options don't need it. I think this can cause other handlers to receive openFileDialog as an unexpected parameter

I assumed when the download option is clicked, this.downloadFile() is called with openFileDialog as an argument. If downloadFile isn't written to handle parameters same applies to removeFile.

@select="(option) => option.onClick(openFileDialog)"
/>
</template>
</KIconButton>
</VListTileAction>
</VListTile>
</FileDropzone>
Expand All @@ -75,7 +72,6 @@
import { mapGetters } from 'vuex';
import FileStatusText from 'shared/views/files/FileStatusText';
import Uploader from 'shared/views/files/Uploader';
import IconButton from 'shared/views/IconButton';
import { constantsTranslationMixin, fileSizeMixin, fileStatusMixin } from 'shared/mixins';
import FileDropzone from 'shared/views/files/FileDropzone';

Expand All @@ -85,7 +81,6 @@
Uploader,
FileDropzone,
FileStatusText,
IconButton,
},
mixins: [constantsTranslationMixin, fileSizeMixin, fileStatusMixin],
props: {
Expand Down Expand Up @@ -150,6 +145,36 @@
}
return null;
},
previewFilesOptions() {
const options = [
{
label: this.$tr('replaceFileMenuOptionLabel'),
value: 'REPLACE_FILE',
onClick: replaceFile => {
replaceFile();
},
condition: this.fileDisplay,
},
{
label: this.$tr('downloadMenuOptionLabel'),
value: 'DOWNLOAD_FILE',
onClick: () => {
this.downloadFile();
},
condition: this.fileDisplay,
},
{
label: this.$tr('removeMenuOptionLabel'),
value: 'REMOVE_FILE',
onClick: () => {
this.removeFile();
},
condition: this.fileDisplay && this.allowFileRemove,
},
];

return options.filter(option => option.condition);
},
},
watch: {
'file.id': {
Expand All @@ -167,11 +192,20 @@
uploadingHandler(fileUpload) {
this.fileUploadId = fileUpload.id;
},
downloadFile() {
window.open(this.fileDisplay.url, '_blank');
},
removeFile() {
this.$emit('remove', this.file);
},
},
$trs: {
uploadButton: 'Select file',
removeFileButton: 'Remove',
replaceFileMenuOptionLabel: 'Replace file',
downloadMenuOptionLabel: 'Download',
removeMenuOptionLabel: 'Remove',
/* eslint-disable kolibri/vue-no-unused-translations */
removeFileButton: 'Remove',
retryUpload: 'Retry upload',
uploadFailed: 'Upload failed',
unknownFile: 'Unknown filename',
Expand All @@ -183,36 +217,32 @@

<style lang="less" scoped>

.layout .section-header {
padding: 0 15px;
font-weight: bold;
color: var(--v-darken-3);
}

button {
margin: 0;
}

/deep/ .v-list__tile {
height: max-content !important;
min-height: 64px;
padding: 5px 16px;

.remove-icon {
display: none;
}

&:hover .remove-icon {
display: block;
&:focus {
background-color: var(--v-grey-lighten5);
outline-color: var(--v-primary-base);
}

.v-list__tile__title {
height: max-content;
height: 30px;
}

.v-list__tile__sub-title {
margin-left: 1px;
white-space: unset;
}
}

.file-display {
margin-left: 1px;

span {
font-size: 15px;
}
}

</style>
28 changes: 10 additions & 18 deletions ...uration/contentcuration/frontend/channelEdit/views/files/__tests__/fileUploadItem.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { factory } from '../../../store';
import Uploader from 'shared/views/files/Uploader';

const testFile = { id: 'test' };
function makeWrapper(props = {}, file = {}) {
function makeWrapper(props = {}, file = {}, computed = {}) {
const store = factory();
return mount(FileUploadItem, {
store,
Expand All @@ -24,6 +24,7 @@ function makeWrapper(props = {}, file = {}) {
},
...props,
},
computed,
});
}

Expand All @@ -34,22 +35,22 @@ describe('fileUploadItem', () => {
original_filename: 'file',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.find('[data-test="file-link"]').text()).toBe('Unknown filename');
expect(wrapper.find('[data-test="file-name"]').text()).toBe('Unknown filename');
});
it("'Unknown filename' should be displayed if original_filename is ''", () => {
const file = {
original_filename: '',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.find('[data-test="file-link"]').text()).toBe('Unknown filename');
expect(wrapper.find('[data-test="file-name"]').text()).toBe('Unknown filename');
});
it("original_filename should be displayed if its value is not 'file'", () => {
const file = {
id: 'file-1',
original_filename: 'SomeFileName',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.find('[data-test="file-link"]').text()).toBe('SomeFileName');
expect(wrapper.find('[data-test="file-name"]').text()).toBe('SomeFileName');
});
it('should show a status error if the file has an error', () => {
const wrapper = makeWrapper({}, { error: true });
Expand All @@ -60,17 +61,13 @@ describe('fileUploadItem', () => {
expect(wrapper.find('[data-test="upload-link"]').exists()).toBe(true);
expect(wrapper.find('[data-test="radio"]').exists()).toBe(false);
});
});

describe('props', () => {
it('should show the remove icon only if allowFileRemove', () => {
const noRemoveWrapper = makeWrapper();
expect(noRemoveWrapper.find('[data-test="remove"]').exists()).toBe(false);

const allowRemoveWrapper = makeWrapper({ allowFileRemove: true });
expect(allowRemoveWrapper.find('[data-test="remove"]').exists()).toBe(true);
it('should show dropdown on click preview file options', () => {
const wrapper = makeWrapper({ allowFileRemove: true });
wrapper.find('[data-test="show-file-options"]').trigger('click');
expect(wrapper.find('[data-test="file-options"]').isVisible()).toBe(true);
});
});

describe('methods', () => {
let wrapper;
beforeEach(() => {
Expand Down Expand Up @@ -104,10 +101,5 @@ describe('fileUploadItem', () => {
wrapper.find('[data-test="list-item"]').trigger('click');
expect(wrapper.emitted('selected')).toBeUndefined();
});
it('clicking remove icon should emit a remove event', () => {
wrapper.setProps({ allowFileRemove: true });
wrapper.find('[data-test="remove"]').vm.$emit('click');
expect(wrapper.emitted('remove')[0][0].id).toBe('test');
});
});
});
2 changes: 1 addition & 1 deletion contentcuration/contentcuration/frontend/shared/views/ActionLink.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
height: unset;
min-height: min-content;
padding: 0;
margin: 0;
margin: 0 0 0 1px;
font-weight: normal;
text-transform: none;
}
Expand Down