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

Revamp content file mgt #4872

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

Conversation

akolson
Copy link
Member

@akolson akolson commented Jan 16, 2025

Summary

This pr revamps tthe content file management UI as described in the figma designs

Before:
image

After:

Screen.Recording.2025-01-29.at.22.49.20.mov

References

Closes #4840

Reviewer guidance

  • Upload a file to a particular folder in a channel
  • Edit the channel
  • Add a different type of the file by clicking the "Select file" link
  • Use the horizontal menu options to replace, remove and download the attached files
  • Use keyboard navigation to perform the same actions above
  • Observe the UI changes. They should match the design spec

@@ -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

@@ -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?

<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.

@AllanOXDi
Copy link
Member

@akolson Great work here. I just left a few comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp content file management in the edit modal
2 participants