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

fix(core): updates to not show "cannot upload" on hover #5881

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

jtpetty
Copy link
Contributor

@jtpetty jtpetty commented Feb 29, 2024

when using extension based accepts settings

Fixes EDX-795

Description

When configuring a file input that restricts file types, we accept either extensions '.pdf' or mime type 'application/pdf'. When using drag and drop, while hovering, if the file type is not accepted, we display an error message. In order for the extension based restriction to work, we need the file name, which is not passed by the browser when hovering, but is when the file is dropped. Because of this, when an extension rule was used, we were incorrectly showing the error message but then accepting the file when it was dropped. The correct fix is to update the rule to be based on mime type. For the case where extension rules are used, we should not show an error on hover.

Before:

Screen.Recording.2024-02-29.at.2.35.05.PM.mov

After:

Screen.Recording.2024-02-29.at.2.31.59.PM.mov

What to review

Are there any side effects of this change that I did not consider?

Testing

Manually tested

Notes for release

@jtpetty jtpetty requested a review from a team as a code owner February 29, 2024 19:43
@jtpetty jtpetty requested review from cngonzalez and removed request for a team February 29, 2024 19:43
Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Feb 29, 2024 7:43pm
test-studio ✅ Ready (Inspect) Visit Preview Feb 29, 2024 7:43pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Feb 29, 2024 7:43pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

Component Testing Report Updated Feb 29, 2024 7:51 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 13s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 1s 15 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 0s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 19s 9 0 0

if (fileName) {
return fileName.toLowerCase().endsWith(validType)
}
// If we do not have a valid fileName and validType is an extension, we
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me!

nit: I had to take a little time to understand what was going on here. If I understand correctly, we pass true here to not get an error state since the filename is not available. However, when the file is dropped, the accept rule will be enforced later on and an invalid type will not be accepted?

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 is correct!

@jtpetty jtpetty added this pull request to the merge queue Mar 1, 2024
Merged via the queue into next with commit 023e7e6 Mar 1, 2024
40 checks passed
@jtpetty jtpetty deleted the edx-795 branch March 1, 2024 16:44
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.

None yet

2 participants