Fix #5200: Fix the unnecessary highlight on the Week Number when the first day of the week is selected using Keyboard #5232
Conversation
![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 for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@balajis-qb you can click here to see the review status or cancel the code review job.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5232 +/- ##
=======================================
Coverage 96.89% 96.89%
=======================================
Files 29 29
Lines 3381 3381
Branches 1401 1414 +13
=======================================
Hits 3276 3276
Misses 105 105 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Code change looks good, there may be some opportunities to improve test coverage and remove dead code
Reviewed with ❤️ by PullRequest
src/test/week_number_test.test.tsx
Outdated
| @@ -383,59 +368,6 @@ describe("WeekNumber", () => { | |||
| ), | |||
| ).toBe(false); | |||
| }); | |||
|
|
|||
| it("should have the class 'react-datepicker__week-number--selected' if selected is not current week and preselected is current week", () => { | |||
There was a problem hiding this comment.
Thank you for this catch. As per your suggestion, I retained test cases of react-datepicker__week-number--selected.
| @@ -120,8 +120,6 @@ export default class WeekNumber extends Component<WeekNumberProps> { | |||
| "react-datepicker__week-number--clickable": !!onClick, | |||
| "react-datepicker__week-number--selected": | |||
| !!onClick && isSameDay(this.props.date, this.props.selected), | |||
| "react-datepicker__week-number--keyboard-selected": | |||
There was a problem hiding this comment.
Removed the CSS part of react-datepicker__week-number--keyboard-selected.
balajis-qb
force-pushed
the
issue-5200/fix/week-number-highlight
branch
from
ce8508f to
ceebc36
Compare
… for the <WeekNumber /> component - Removed as highlighting the first day of the week alone highlights the week number - Because we set the date prop of the <WeekNumber /> to first date of the week. As a result this highlight will work only for the first day of week
balajis-qb
force-pushed
the
issue-5200/fix/week-number-highlight
branch
from
ceebc36 to
9f86dd4
Compare
|
Hi, |
Closes #5200
Description
As mentioned in the ticket, when
showWeekNumbersis enabled in a datepicker, the keyboard select on the first day of the week alone highlights the week number. When we select any other week days it's not highlighting. This issue is not occuring in the mouse hover of any week days.Why the issue occurs?
As I shared in the above screenshot, we passed the start of the week to each corresponding week number and in the
<WeekNumber />component we apply a classreact-datepicker__week-number--keyboard-selectedwhen the date prop (which is start of the week) is selected.Changes
react-datepicker__week-number--keyboard-selectedto . The reason is because we are not applying any class to it when we hover the first day of the week using mouse. Also if we plan to highlight the week number it should enabled for all week days and not just the start date and it should be applied to the mouse hover aswell. But I don't think that's necessary as the week number we're displaying is just looks like a read-only kind of view, as it's grayed out currently. Hence I feel removing the classreact-datepicker__week-number--keyboard-selectedwould be a better way to fix the issue.Contribution checklist