-
Notifications
You must be signed in to change notification settings - Fork 65
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 Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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?
| 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?
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
050d43c to
5e64bc4
Compare
5e64bc4 to
a5b7748
Compare
No description provided.