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
Read-only is not working in file-attachment #1200
Conversation
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
208825c
to
ded2602
Compare
ded2602
to
dcb2a9c
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
4 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1200 +/- ##
=========================================
Coverage 80.81% 80.81%
Complexity 782 782
=========================================
Files 91 91
Lines 2116 2116
Branches 286 286
=========================================
Hits 1710 1710
Misses 252 252
Partials 154 154 ☔ View full report in Codecov by Sentry. |
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.
please add test cases for all the changes
dcb2a9c
to
1912845
Compare
1912845
to
c619810
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
@@ -86,6 +86,19 @@ | |||
return this.element.querySelector(FileInputV2.selectors.attachButtonLabel); | |||
} | |||
|
|||
updateEnabled(enabled, state) { |
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.
updateEnabled(enabled, state) {
// Update data attribute for enabled state
this.element.setAttribute(FormView.Constants.DATA_ATTRIBUTE_ENABLED, enabled);
// Determine if the widget should be disabled
const isDisabled = !enabled || state.readOnly;
// Update the 'disabled' and ARIA 'disabled' attributes based on the enabled state
if (isDisabled) {
this.widget.setAttribute("disabled", "disabled");
this.widget.setAttribute(FormView.Constants.ARIA_DISABLED, "true");
} else {
this.widget.removeAttribute("disabled");
this.widget.removeAttribute(FormView.Constants.ARIA_DISABLED);
}
}
Always try to create a constant inside the if condition to make the code more readable
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.
sure, implemented the same
@@ -84,7 +84,6 @@ | |||
} | |||
|
|||
updateEnabled(enabled, state) { | |||
this.toggle(enabled, FormView.Constants.ARIA_DISABLED, true); |
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.
Why is this code removed ? Isn't this backward incompatible
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.
This line was adding aria-disabled attribute to the parent div which is of no use, so I had removed this line.
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.
check comments
c619810
to
6ae4ae0
Compare
6ae4ae0
to
40371fb
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
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.
check comments
@@ -80,7 +80,7 @@ describe("Form with File Input - Basic Tests", () => { | |||
const checkHTML = (id, state) => { | |||
const visible = state.visible; | |||
const passVisibleCheck = `${visible === true ? "" : "not."}be.visible`; | |||
const passDisabledAttributeCheck = `${state.enabled === false ? "" : "not."}have.attr`; | |||
const passDisabledAttributeCheck = `${state.enabled === false || state.readOnly === true ? "" : "not."}have.attr`; |
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.
As discussed in the scrum, can you please add the same check in all the test specs for all the components ?
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.
Sure Rishi, Will be adding the test cases in the next pr which I am working on for tnc component
Co-authored-by: Pavitra Khatri <pavitrakhatri@pavitras-mbp.corp.adobe.com>
Co-authored-by: Pavitra Khatri <pavitrakhatri@pavitras-mbp.corp.adobe.com>
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: