fix(DatePicker): fixed disabled state for DatePicker - #21848
fix(DatePicker): fixed disabled state for DatePicker#21848suniltc-ibm wants to merge 24 commits into
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21848 +/- ##
==========================================
+ Coverage 89.62% 90.26% +0.64%
==========================================
Files 582 265 -317
Lines 53317 40045 -13272
Branches 7312 2210 -5102
==========================================
- Hits 47784 36148 -11636
+ Misses 5396 3879 -1517
+ Partials 137 18 -119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adamalston
left a comment
There was a problem hiding this comment.
Could you add testing?
|
@adamalston Updated code and covered test. Please check. |
| const prefix = usePrefix(); | ||
| const normalizedProps: NormalizedInputProps = { | ||
| disabled: !readOnly && disabled, | ||
| disabled: disabled, |
There was a problem hiding this comment.
The useNormalizedInputProps hook is used in 11 components. Should this change in precedence be limited to DatePickerInput, or should it also be applied to other consumers of this hook, with corresponding updates to their styles and tests? With the current code, it appears that the TextInput, Select, Dropdown, and the TimePicker input will all render in a read-only state when both props are set to true.
There was a problem hiding this comment.
It's applied to all the components. It fixes the same issue in other components as well.
There was a problem hiding this comment.
@adamalston Can you please review it again
There was a problem hiding this comment.
Sure. In the future, re-requesting a review is the most effective way to ensure a pull request appears in my queue.
There was a problem hiding this comment.
Test coverage for this change was added only for DatePicker. Could you also add coverage for the other affected components?
|
Hi @suniltc-ibm , thanks for contributing! Could you also add coverage for the other affected components? There are also some merge conflicts that need to be resolved. |
Co-authored-by: Adam Alston <aalston9@gmail.com>
03ab0dc to
ecc786a
Compare
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| expect(select.hasAttribute('disabled')).to.be.true; | ||
| expect(select.hasAttribute('readonly')).to.be.false; | ||
| expect(select.disabled).to.be.true; | ||
| expect(select.readOnly).to.be.false; |
There was a problem hiding this comment.
With tests failing, I think this should assert aria-readonly instead?
✅ Deploy Preview for v3-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v2-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| const labelClasses = classNames(`${prefix}--label`, { | ||
| [`${prefix}--visually-hidden`]: hideLabel, | ||
| [`${prefix}--label--disabled`]: normalizedProps.disabled, | ||
| [`${prefix}--label--readonly`]: readOnly, |
There was a problem hiding this comment.
${prefix}--label--readonly is still applied when disabled is also true, so the label keeps the read-only styling even though the input now treats disabled as higher precedence. Is that correct?
There was a problem hiding this comment.
@adamalston Thanks for pointing this out. I didn't notice any issue with it earlier, so I had skipped it. I've now updated the code to include the !disabled guard.
| const labelClasses = classNames(`${prefix}--label`, { | ||
| [`${prefix}--visually-hidden`]: hideLabel, | ||
| [`${prefix}--label--disabled`]: disabled, | ||
| [`${prefix}--label--readonly`]: readOnly, |
There was a problem hiding this comment.
${prefix}--label--readonly is still applied when disabled and readOnly are both true, which leaves the label in the old mixed state after the input behavior changed. Is that correct?
| const labelClasses = cx(`${prefix}--label`, { | ||
| [`${prefix}--visually-hidden`]: hideLabel, | ||
| [`${prefix}--label--disabled`]: disabled, | ||
| [`${prefix}--label--readonly`]: readOnly, |
| const className = cx({ | ||
| [`${prefix}--label`]: true, | ||
| [`${prefix}--label--disabled`]: disabled, | ||
| [`${prefix}--label--readonly`]: readOnly, |
| ); | ||
| const titleClasses = cx(`${prefix}--label`, { | ||
| [`${prefix}--label--disabled`]: disabled, | ||
| [`${prefix}--label--readonly`]: readOnly, |
Closes #21803
DatePicker: disabled state ignored when readOnly is also true
Changelog
Changed
Testing / Reviewing
Navigate to Date Picker
Click on Default story
Set Disabled control to true
Set Read Only to true
Disabled overwrites Read Only
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
More details can be found in the pull request guide