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

File Dropzone: Accepts Correct and Rejects Incorrect File Types #2750

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

diTrimInt
Copy link
Contributor

@diTrimInt diTrimInt commented Aug 3, 2024

Description

References #2695

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

Test Cases
All test cases work for both when dragging or uploading files.

Test Case#1 - Accepting one File Type

Component Attributes For Test

      <modus-file-dropzone
        aria-Label="dropzone"
        description="File dropzone description"
        dropzone-Height="175px"
        dropzone-Width="400px"
        label="Dropzone Label"
        multiple="false"
        accept-file-types=".jpeg">
      </modus-file-dropzone>

<script>
    const fileDropzone = document.querySelector('modus-file-dropzone');
    const fileList = document.querySelector('.file-list');

    fileDropzone.addEventListener('files', (event) => {
      const [files, error] = event.detail;

      fileList.innerHTML = '';

      if (files.length > 0) {
        // Display the list of uploaded files
        files.forEach((file) => {
          const li = document.createElement('li');
          li.textContent = file.name;

          const removeButton = document.createElement('button');
          removeButton.textContent = 'Remove';

          removeButton.addEventListener('click', () => {
            fileDropzone.removeFile(file.name);
          });

          li.appendChild(removeButton);
          fileList.appendChild(li);
        });
      }

      if (error) {
        console.error('Error:', error);
      }
    });
  </script>
1.mp4

Test Case#2 - Accepting Multiple Types

Component Attributes For Test

      <modus-file-dropzone
        aria-Label="dropzone"
        description="File dropzone description"
        dropzone-Height="175px"
        dropzone-Width="400px"
        label="Dropzone Label"
        max-file-count="4"
        accept-file-types=".jpeg,.png">
      </modus-file-dropzone>
2.mp4

Test Case#3 - Accepts Anything

<modus-file-dropzone
      aria-Label="dropzone"
      description="File dropzone description"
      dropzone-Height="175px"
      dropzone-Width="400px"
      label="Dropzone Label"
      max-file-count="4">
    </modus-file-dropzone>
3-REAL.mp4

Test Case#4 - Accepts image/png,.jpeg

 <modus-file-dropzone
      aria-Label="dropzone"
      description="File dropzone description"
      dropzone-Height="175px"
      dropzone-Width="400px"
      label="Dropzone Label"
      multiple="false"
      accept-file-types="image/png,.jpeg">
    </modus-file-dropzone>
3.mp4

Test Case#5 - Accepts image/*

 <modus-file-dropzone
      aria-Label="dropzone"
      description="File dropzone description"
      dropzone-Height="175px"
      dropzone-Width="400px"
      label="Dropzone Label"
      multiple="false"
      accept-file-types="image/*">
    </modus-file-dropzone>
5.mp4

Test Case#6 - Accepts image/png, application/pdf, .jpeg*

      <modus-file-dropzone
      aria-Label="dropzone"
      description="File dropzone description"
      dropzone-Height="175px"
      dropzone-Width="400px"
      label="Dropzone Label"
      accept-file-types="image/png,application/pdf,.jpeg">
    </modus-file-dropzone>
6.mp4

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (Note: I listed three functional test cases above.)
  • New and existing unit tests pass locally with my changes (Note: No new unit test have been added)
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link

netlify bot commented Aug 3, 2024

Deploy Preview for moduswebcomponents ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7527e99
🔍 Latest deploy log https://app.netlify.com/sites/moduswebcomponents/deploys/66e4e60dfe76cf0008f46f16
😎 Deploy Preview https://deploy-preview-2750--moduswebcomponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 23 (🔴 down 1 from production)
Accessibility: 98 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@diTrimInt diTrimInt changed the title Accepts Correct and Rejects Incorrect File Types File Dropzone: Accepts Correct and Rejects Incorrect File Types Aug 6, 2024
Copy link
Contributor

@prashanth-offcl prashanth-offcl left a comment

Choose a reason for hiding this comment

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

@diTrimInt Thanks for the detailed PR description. I tested your code and found a bug, could you please resolve it?

The accept-types property is mapped to file input type accept so we should also consider other possible valid values of accept like images/*, application/msword. currently it working great if I pass file extensions and break if pass other valid values.

Adding reference link
https://github.com/react-dropzone/attr-accept/blob/66260177829f99c48594dd1fdedaefbb91bdce39/src/index.js#L12

@diTrimInt
Copy link
Contributor Author

@prashanth-offcl Thank you for letting me know. I will work on adding the logic needed to account for other possible valid values of accept like images/*, application/msword.

@prashanth-offcl prashanth-offcl marked this pull request as draft August 30, 2024 15:21
@diTrimInt
Copy link
Contributor Author

@prashanth-offcl I made the changes to account for other possible valid values of accept like images/*, application/msword. Please let me know if there are anymore needed changes so I can make it before my internship ends 9/20/2024.

@prashanth-offcl prashanth-offcl marked this pull request as ready for review September 4, 2024 10:09
@prashanth-offcl
Copy link
Contributor

@diTrimInt Looks like the comma separated extension (.doc,.docx) scenario is broken now, and the implemented behavior is consistent with the existing ones. In the existing behavior if there are any errors we don't remove the files directly, we just notify the error to the consumer and let them remove it by calling removeFile method, but the new implementation directly removes the files without notifying the consumer. Can you handle this?

@prashanth-offcl prashanth-offcl marked this pull request as draft September 11, 2024 10:53
@diTrimInt
Copy link
Contributor Author

@prashanth-offcl Thank you for pointing this out. I'll get to work on it.

@diTrimInt
Copy link
Contributor Author

@prashanth-offcl Please let me know if there is anything left to correct on this. If I don't have time to make any changes before my internship ends, I can close out this PR, clone the repository on my personal GitHub, and continue to work on it from there. FYI, I would still like to keep contributing after my internship ends from my personal GitHub account.

@diTrimInt diTrimInt marked this pull request as ready for review September 14, 2024 01:31
return;
}

if (this.error === 'invalidFileType' && invalidFiles.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@diTrimInt if there are no errors then the error variables are reset at the end of this function so we can remove this

}
return acceptedType === fileType;
} else {
return '.' + fileExtension === acceptedType;
Copy link
Contributor

Choose a reason for hiding this comment

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

@diTrimInt I'm still able to reproduce the issue with comma separated extensions (.doc,.docx,.jpg,.png). Can we have a separate check for . like this

Components._.File.Dropzone.-.Default.Storybook.-.Google.Chrome.2024-09-17.15-48-22.mp4


if (invalidFiles.length > 0) {
this.error = 'invalidFileType';
this.errorMessageTop = `Some files are not of the accepted types. Please remove the following file(s) to continue: ${invalidFiles
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll check with our UX and get back on how to handle this specific error state with overflowing content.

Copy link

Choose a reason for hiding this comment

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

Hi @prashanth-offcl, It's Dion. I will continue the work for my personal Github account (this one). I wanted to follow-up on this comment to see if UX's response for this.

@prashanth-offcl prashanth-offcl marked this pull request as draft September 17, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants