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

feat(file-picker): support clearing files (VIV-1995) #1983

Merged
merged 5 commits into from
Nov 11, 2024
Merged

feat(file-picker): support clearing files (VIV-1995) #1983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5613ba9
Support removing all files
RichardHelm Nov 6, 2024
f3f0145
Revert dropzone back to private
Nov 7, 2024
ec79569
Merge branch 'main' into VIV-1995-clearing-files
YonatanKra Nov 8, 2024
d3266d1
Merge branch 'main' into VIV-1995-clearing-files
RichardHelm Nov 8, 2024
58e08dc
Merge branch 'main' into VIV-1995-clearing-files
RichardHelm Nov 11, 2024
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
10 changes: 10 additions & 0 deletions libs/components/src/lib/file-picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,16 @@ If needed, the background of the item can be changed using the `--file-picker-li

</div>

## Methods

<div class="table-wrapper">

| Name | Returns | Description |
| ------------------ | ------- | --------------------------------------- |
| `removeAllFiles()` | `void` | Removes all files from the File Picker. |

</div>

## Use Cases

### In a form
Expand Down
26 changes: 23 additions & 3 deletions libs/components/src/lib/file-picker/file-picker.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ describe('vwc-file-picker', () => {
unmountedElement.maxFileSize = 256;
unmountedElement.maxFiles = 1;
unmountedElement.accept = '.jpg';
unmountedElement.removeAllFiles();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't removeAllFiles tested in its own it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just to cover the optional chain.
To ensure it doesn't throw when component is unmounted

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the full test (for some reason it was hidden in the diff), it's a very general test and an unclear spec.

  1. Would removeAllFiles cause an error in unmounted state at any point?
  2. Did removing and appending the component to the DOM (connecting/disconnecting in WC terms?) cause an error before?
    In other words, what piece of code do I delete for the expectation to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removeAllFiles will throw an error in unmounted state without the optional chain (because dropzone is not initialised)


expect(unmountedElement.files).toEqual([]);
expect(() => {
Expand Down Expand Up @@ -111,6 +112,17 @@ describe('vwc-file-picker', () => {
addFiles([await generateFile('london.png', 1)]);
expect(element.value).toBe('C:\\fakepath\\london.png');
});

it('should remove all files when setting value to an empty string', async function () {
RichardHelm marked this conversation as resolved.
Show resolved Hide resolved
addFiles([
await generateFile('london.png', 1),
await generateFile('london.png', 1),
]);

element.value = '';

expect(element.files.length).toBe(0);
});
});

describe('max file size', function () {
Expand Down Expand Up @@ -313,6 +325,17 @@ describe('vwc-file-picker', () => {
});
});

describe('removeAllFiles()', function () {
it('should remove all files', async function () {
addFiles([
await generateFile('london.png', 1),
await generateFile('london.png', 1),
]);
element.removeAllFiles();
expect(element.files.length).toEqual(0);
});
});

describe('choose file with keyboard', function () {
it.each([
['Space', ' '],
Expand Down Expand Up @@ -446,6 +469,3 @@ describe('form associated vwc-file-picker', function () {
expect(new FormData(formElement).get('file')).toBeTruthy();
});
});

// Add docs
// Check to see if we have a ui-test that needs update
67 changes: 44 additions & 23 deletions libs/components/src/lib/file-picker/file-picker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,25 @@ const generateFilePreviewTemplate = (
@errorText
@formElements
export class FilePicker extends FormAssociatedFilePicker {
#dropzone?: Dropzone;
/**
* @internal
*/
_dropzone?: Dropzone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to expose dropzone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise there is an error when accessing #dropzone from valueChanged
I believe this is because FAST will call valueChanged before the constructor is completed and the private member is not ready

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a change that reverts it. Seems to work.
The problem is that valuechanged is sent to an event listener, which fires outside the context of the element.
By changing the valueChanged to be an arrow function, it retains the context wherever it is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not what's happening though. With your fix valueChange will be initialised at a later point and miss the initial call that causes the error. But I'm happy to accept it like this


/**
* Files that have been added to the file picker and passed validation.
*
* @public
*/
get files(): File[] {
return this.#dropzone?.getAcceptedFiles() ?? [];
return this._dropzone?.getAcceptedFiles() ?? [];
}

#syncSingleFileState() {
if (this.singleFile) {
this.#dropzone?.hiddenFileInput?.removeAttribute('multiple');
this._dropzone?.hiddenFileInput?.removeAttribute('multiple');
} else {
this.#dropzone?.hiddenFileInput?.setAttribute('multiple', 'multiple');
this._dropzone?.hiddenFileInput?.setAttribute('multiple', 'multiple');
}
}
/**
Expand All @@ -89,11 +92,11 @@ export class FilePicker extends FormAssociatedFilePicker {
*/
@attr({ attribute: 'max-files' }) maxFiles?: number;
maxFilesChanged(_oldValue: number, newValue: number): void {
if (!this.#dropzone) {
if (!this._dropzone) {
return;
}

this.#dropzone.options.maxFiles = newValue;
this._dropzone.options.maxFiles = newValue;
this.#updateHiddenFileInput();
}

Expand All @@ -107,11 +110,11 @@ export class FilePicker extends FormAssociatedFilePicker {
@attr({ mode: 'fromView', attribute: 'max-file-size' })
maxFileSize = 256;
maxFileSizeChanged(_oldValue: number, newValue: number): void {
if (!this.#dropzone) {
if (!this._dropzone) {
return;
}

this.#dropzone.options.maxFilesize = newValue;
this._dropzone.options.maxFilesize = newValue;
}

/**
Expand All @@ -123,11 +126,11 @@ export class FilePicker extends FormAssociatedFilePicker {
*/
@attr accept?: string;
acceptChanged(_oldValue: string, newValue: string): void {
if (!this.#dropzone) {
if (!this._dropzone) {
return;
}

this.#dropzone.options.acceptedFiles = newValue;
this._dropzone.options.acceptedFiles = newValue;
}

/**
Expand All @@ -144,6 +147,17 @@ export class FilePicker extends FormAssociatedFilePicker {
this.#updateFormValue();
}

/**
* @internal
*/
override valueChanged(previous: string, next: string) {
super.valueChanged(previous, next);
// Like input[type=file], remove files when value is set to empty string
if (next === '' && this.files.length) {
this.removeAllFiles();
}
}

/**
* @internal
*/
Expand Down Expand Up @@ -175,15 +189,15 @@ export class FilePicker extends FormAssociatedFilePicker {
};

#localizeFileSizeNumberAndUnits = () => {
(this.#dropzone as any).filesize = (size: number) => {
(this._dropzone as any).filesize = (size: number) => {
return this.#formatNumbersInMessage(
(Dropzone.prototype as any).filesize.call(this.#dropzone, size)
(Dropzone.prototype as any).filesize.call(this._dropzone, size)
);
};
};

#addRemoveButtonToFilesPreview() {
this.#dropzone!.on('addedfiles', (files) => {
this._dropzone!.on('addedfiles', (files) => {
for (const file of files) {
if (file.previewElement) {
const removeButton = file.previewElement.querySelector(
Expand All @@ -192,7 +206,7 @@ export class FilePicker extends FormAssociatedFilePicker {
removeButton.addEventListener('click', (e) => {
e.preventDefault();
e.stopPropagation();
this.#dropzone!.removeFile(file as File as DropzoneFile);
this._dropzone!.removeFile(file as File as DropzoneFile);
});
}
}
Expand All @@ -204,7 +218,7 @@ export class FilePicker extends FormAssociatedFilePicker {
}

#setRemoveButtonConnotationOnError() {
this.#dropzone!.on('error', (file) => {
this._dropzone!.on('error', (file) => {
if (file.previewElement) {
const removeButton = file.previewElement.querySelector(
'.remove-btn'
Expand All @@ -223,7 +237,7 @@ export class FilePicker extends FormAssociatedFilePicker {
const previewList = this.shadowRoot!.querySelector(
'.preview-list'
) as HTMLDivElement;
this.#dropzone = new Dropzone(control, {
this._dropzone = new Dropzone(control, {
url: '/', // dummy url, we do not use dropzone's upload functionality
maxFiles: this.maxFiles ?? (null as any),
maxFilesize: this.maxFileSize,
Expand All @@ -243,7 +257,7 @@ export class FilePicker extends FormAssociatedFilePicker {

this.#addRemoveButtonToFilesPreview();

this.#dropzone.on('removedfile', () => {
this._dropzone.on('removedfile', () => {
this.#handleFilesChanged();
});

Expand All @@ -252,7 +266,7 @@ export class FilePicker extends FormAssociatedFilePicker {

override disconnectedCallback() {
super.disconnectedCallback();
this.#dropzone!.destroy();
this._dropzone!.destroy();
}
/**
* Used internally to set the button tag.
Expand All @@ -273,20 +287,20 @@ export class FilePicker extends FormAssociatedFilePicker {
}

#chooseFile(): void {
if (this.#dropzone!.hiddenFileInput) {
this.#dropzone!.hiddenFileInput.click();
if (this._dropzone!.hiddenFileInput) {
this._dropzone!.hiddenFileInput.click();
}
}

#updateHiddenFileInput(): void {
this.#dropzone!.hiddenFileInput!.dispatchEvent(
this._dropzone!.hiddenFileInput!.dispatchEvent(
new Event('change', { bubbles: false })
);
}

#keepOnlyNewestFile() {
for (let i = 0; i < this.files.length - 1; i++) {
this.#dropzone!.removeFile(this.files[i] as File as DropzoneFile);
this._dropzone!.removeFile(this.files[i] as File as DropzoneFile);
}
}

Expand Down Expand Up @@ -337,7 +351,7 @@ export class FilePicker extends FormAssociatedFilePicker {

override formResetCallback(): void {
super.formResetCallback();
this.#dropzone!.removeAllFiles();
this._dropzone!.removeAllFiles();
}

#formatNumbersInMessage(message: string) {
Expand All @@ -346,6 +360,13 @@ export class FilePicker extends FormAssociatedFilePicker {
}
return message;
}

/**
* Removes all files from the File Picker.
*/
removeAllFiles() {
this._dropzone?.removeAllFiles();
}
}

export interface FilePicker
Expand Down
Loading