-
Notifications
You must be signed in to change notification settings - Fork 355
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
Datepicker Modal Dialog: Fixed next/prev month bug and improve adherence with APG code style guide #2643
Conversation
… behavior for dates near end of month (pull #2618) Provides partial resolution of #2581. The remainder of the resolution is provided by #2643. This PR changes behavior of commands for next and previous month and year In the combobox date picker when the currently focused date does not exist in the previous or next month or year. With these changes, if the same day number does not exist in the newly displayed month, the last day of the newly displayed month receives focus. --------- Co-authored-by: Matt King <a11yThinker@gmail.com>
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Subtopic: PR 2643 - Datepicker Modal Dialog: Updated code and fixed next/prev month bug by jongund<jugglinmike> github: https://github.com//pull/2643 <jugglinmike> Matt_King: I'll be doing the editorial review <jugglinmike> Matt_King: I'm looking for support on visual design review, code review, and testing <jugglinmike> Matt_King: There aren't a lot of changes in this pull request; it's only four files (one CSS and two JS and on HTML) <jugglinmike> Matt_King: Because there was a small change to the CSS, I want to make sure someone is looking at this visually <jugglinmike> arigilmore: I can do visual review and code review <jugglinmike> Matt_King: Great--thank you <jugglinmike> Alex_Flenniken: I can do the test review <jugglinmike> Matt_King: arigilmore do you think you'll be able to do that review this week? <jugglinmike> arigilmore: Yes |
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> subtopic: PR 2643 - Datepicker Modal Dialog: Updated code and fixed next/prev month bug by jongund<jugglinmike> github: https://github.com//pull/2643 <jugglinmike> Matt_King: It looks like arigilmore_ has done her piece <jugglinmike> Matt_King: Looks like it's just for me and Alex to wrap up |
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.
Functional and editorial review complete. Good to go.
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Subtopic: PR 2643 - Datepicker Modal Dialog: Updated code and fixed next/prev month bug by jongund<jugglinmike> github: https://github.com//pull/2643 <jugglinmike> Matt_King: I think this is really close <jugglinmike> Matt_King: The only thing we're waiting on is a review of the regression tests by Alex_Flenniken <jugglinmike> jugglinmike: That will probably not happen this week due to Bocoup's week-long "all-hands" meeting taking place now |
@alflennik would it be possible for you to review Datepicker Modal Dialog update so that we can merge this? |
Hi @a11ydoer , Alex had an issue running the test locally and will need additional time to do this review so we will have to put it into our triaging processes. I will provide an update next week. |
); | ||
}; | ||
|
||
const checkDate = async function assertAriaRoles(t, dateSelector, day) { |
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.
const checkDate = async function assertAriaRoles(t, dateSelector, day) { | |
const checkDate = async function (t, dateSelector, day) { |
This line creates a function named assertAriaRoles
and stores it in a variable named checkDate
. The name is somewhat confusing in this context because this file already has a function stored in the variable assertAriaRoles
.
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.
@jugglinmike
Thank you for the feedback, I updated the code.
}; | ||
|
||
const checkDate = async function assertAriaRoles(t, dateSelector, day) { | ||
const d = day.getDate() < 10 ? '0' + day.getDate() : day.getDate(); |
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.
const d = day.getDate() < 10 ? '0' + day.getDate() : day.getDate(); | |
const d = String(day.getDate()).padStart(2, '0'); |
The code works as written, so you can take or leave this suggestion (I just feel it communicates your intent better).
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.
@jugglinmike
Thank you for the suggestion, I updated the code.
const startDate = '1/31/2023'; | ||
const targetDates = [ | ||
'1/31/2022', | ||
'1/31/2021', | ||
'1/31/2020', | ||
'1/31/2019', | ||
'1/31/2018', | ||
'1/31/2017', | ||
]; |
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 doesn't demonstrate the bug fix (it passes on the main
branch). Starting from 2020-02-29 would do it, though
const startDate = '1/31/2023'; | |
const targetDates = [ | |
'1/31/2022', | |
'1/31/2021', | |
'1/31/2020', | |
'1/31/2019', | |
'1/31/2018', | |
'1/31/2017', | |
]; | |
const startDate = '2/29/2020'; | |
const targetDates = [ | |
'2/28/2019', | |
'2/28/2018', | |
'2/28/2017', | |
'2/29/2016', | |
'2/28/2015', | |
'2/28/2014', | |
]; |
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.
@jugglinmike
Thank you for the feedback, I updated the code.
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Subtopic: Datepicker Modal Dialog: Fixed next/prev month bug and improve adherence with APG code style guide<jugglinmike> github: https://github.com//pull/2643 <jugglinmike> Matt_King: jongund , about two weeks ago, jugglinmike posted feedback for you on the tests <jugglinmike> jongund: I'll take a look |
@jugglinmike @mcking65 |
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.
The tests look good to @Paul-Clue and me, @jongund! Thanks!
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> subtopic: Datepicker Modal Dialog: Fixed next/prev month bug and improve adherence with APG code style guide<jugglinmike> github: https://github.com//pull/2643 <jugglinmike> jugglinmike: I reviewed and approved the tests with Paul at Bocoup. <jugglinmike> Matt_King: Then I think this is ready to merge |
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.
Thank you @jongund for all your work on this!! This is now ready to ship!
This PR complements #2618 to provide the remaining changes to resolve issue #2581.
This PR changes behavior of commands for next and previous month and year In the date picker dialog when the currently focused date does not exist in the previous or next month or year. With these changes, if the same day number does not exist in the newly displayed month, the last day of the newly displayed month receives focus.
In addition to the above enhancement, this PR also includes the following changes:
class
constructor and pointer eventsPreview updated Date Picker Modal Dialog Example
Review checklist
WAI Preview Link (Last built on Tue, 16 May 2023 19:24:24 GMT).