Fix #4259: Add the aria-disabled on the Time component's disabled time units #4319
Conversation
Moved the logic to find whether a time is disabled logic to a helper method to reuse the logic both for the add of react-datepicker__time-list-item--disabled class ans also for the add of aria-disabled attribute Closes Hacker0x01#4259
…Time component Ensure the test cases to accurately check for the presence of 'aria-disabled' attribute in the Time component when it's disabled Closes Hacker0x01#4259
…times_test.js Migrated the test cases to RTL as mentioned by @martijnrusschen in the issue Hacker0x01#4300 for React 18 upgrade
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@balajis-qb you can click here to see the review status or cancel the code review job.
Codecov Report
@@ Coverage Diff @@
## main #4319 +/- ##
=======================================
Coverage 96.63% 96.63%
=======================================
Files 27 27
Lines 2374 2376 +2
Branches 966 967 +1
=======================================
+ Hits 2294 2296 +2
Misses 80 80
|
There was a problem hiding this comment.
Updates the aria-disabled attribute as described and updates tests accordingly. The code looks proper and reasonable and I see no reasons to prevent this from moving forward
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
I can see the change you have made around aria-disable. I have a suggestion for how to do the tests slightly different so that the code base is easier to work with for the next person. Great to see the tests! Nice work!
Reviewed with ❤️ by PullRequest
| ".react-datepicker__time-list-item--disabled", | ||
| ); | ||
| expect(disabledItems).toHaveLength(45); | ||
| expect(disabledTimeItems.length).toBe(45); |
There was a problem hiding this comment.
This seems like it would make the test have a higher than necessary amount of maintenance. If something is added to the UI it would alter this count .. right? I would recommend writing this in a way that the functionality of the aria-disabled is verified to work. For example, you probably didn't go through and count to make sure that 45 is the right number ... I'm guessing the test failed and saw it was 45?
In summary, I'm suggesting:
- export or move the new
isDisabledTimefunction and write a unit test against that little bit of logic. - Remove the assertions around number of nodes.
- Determine a filter that selects the nodes which should have aria-disabled (
input,[onClick], etc), and write a test that selects those nodes and simply verify that aria-disabled is set. Thus catching new elements which are missing the attribute. Maybe have a list of some kind (or an custom attribute to suppress) that would make an exception if the filter is too broad.
🔹 Best Practices (Nice to have)
Revised test cases to ensure that all the disabled time items have the 'aria-disabled' attribute set.
|
Thanks for the refactor as well, that's making the future tests a lot more future proof. |
Closes #4259
Summary
This PR addresses an issue where the Time component is missing the attribute
aria-disabledon it's disabled items.Changes made
aria-disabledattribute over it's disabled itemsaria-disabledattribute along with the class.react-datepicker__time-list-item--disabledRTLfromenzymeas suggested by @martijnrusschen in the issue Rewrite usage of ReactDOM.render to React 18 #4300 for React 18 upgrade