-
Notifications
You must be signed in to change notification settings - Fork 58
Forms 18927 allow extensions support in file input #1586
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
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1586 +/- ##
============================================
+ Coverage 82.32% 82.34% +0.01%
- Complexity 1000 1004 +4
============================================
Files 108 108
Lines 2603 2611 +8
Branches 370 371 +1
============================================
+ Hits 2143 2150 +7
- Misses 272 273 +1
Partials 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
920a278
to
c10b1e9
Compare
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
mimeTypeField.find('coral-multifield-item:not(:first)').remove(); | ||
|
||
// Set value of first item to */* | ||
firstItem.find('input').val('*/*'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this "firstItem.find('input').val('/');" means all mimetypes are allowed or ignored?
} | ||
|
||
if(mimeTypeFieldInfoIcon.length > 0) { | ||
// show the info icon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Info icon should be present by default, not based on condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajatofficial To confirm - this is need because it is behind FT?
isInvalidFileName = isCurrentInvalidFileName = true; | ||
inValidNamefileNames = currFileName + "," + inValidNamefileNames; | ||
} else { | ||
// Now size is in MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajatofficial why this formatting change - the previous one looks correct..
|
||
// Disable the entire multifield and its contents | ||
mimeTypeField.css({ | ||
'pointer-events': hasExtensions ? 'none' : 'auto', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls manage css by adding relevant classes rather inside the js itself.
mimeTypeField.prop('disabled', hasExtensions); | ||
|
||
// Disable delete icons and their SVGs | ||
mimeTypeField.find('coral-icon[icon="delete"], ._coral-Icon--svg').css({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything that starts with a underscore is an internal coral selector. We should not use any coral internals in the code.
// If no first item exists, create one | ||
if (firstItem.length === 0) { | ||
// Click the add button to create a new item | ||
mimeTypeField.find('button[coral-multifield-add]').click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than simulating clicks we should be using add API for Multi field.
}); | ||
|
||
// Handle MIME type items if extensions exist | ||
if (hasExtensions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not immediately clear from this code that what, why and when do we intend to do this
Is it just to reset the mimetype with just 1 item whose value is */*
whenever extensions exist/added? Im suspecting it's due to avoidance of gng into complexities wrt conflict in server side validation. Am I correct?
@@ -123,6 +123,7 @@ private ReservedProperties() { | |||
public static final String PN_UNCHECKED_VALUE = "uncheckedValue"; | |||
public static final String PN_MAX_FILE_SIZE = "maxFileSize"; | |||
public static final String PN_FILE_ACCEPT = "accept"; | |||
public static final String PN_FILE_ACCEPT_EXTENSIONS = "acceptExtensions"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any new prop persisted in jcr should being with fd:
let hasExtensions = extensionsField.find('coral-multifield-item').length > 0; | ||
if (hasExtensions) { | ||
mimeTypeField.css({ | ||
'pointer-events': 'none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Html5 file accept allows mimetypes and extensions to be set simultaneously. is there any specific reason for this otherwise behavior in our component?
No description provided.