Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(datepicker): aria-label added on days #2319

Merged
merged 1 commit into from
Apr 27, 2018
Merged

feat(datepicker): aria-label added on days #2319

merged 1 commit into from
Apr 27, 2018

Conversation

fbasso
Copy link
Member

@fbasso fbasso commented Apr 16, 2018 •
edited by maxokorokov
Loading

Part of #1946

@maxokorokov maxokorokov self-assigned this Apr 17, 2018
maxokorokov
maxokorokov requested changes Apr 17, 2018
View reviewed changes
Copy link
Member

@maxokorokov maxokorokov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. A couple of comments:

  • some tests are failing on Travis CI
  • could you please update datpicker-service.spec.ts with a couple of unit tests for day.ariaLabel generation?
  • which screen readers have you tested it with?

demo/src/app/components/datepicker/datepicker.component.ts Outdated
@@ -30,16 +30,16 @@ import {DEMO_SNIPPETS} from './demos';
<ngbd-datepicker-disabled></ngbd-datepicker-disabled>
</ngbd-example-box>
<ngbd-example-box demoTitle="Custom date adapter" [snippets]="snippets" component="datepicker" demo="adapter">
<ngbd-datepicker-adapter></ngbd-datepicker-adapter>
<ngbd-datepicker-adapter></ngbd-datepicker-adapter>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please revert changes to datepicker.component.ts, you removed spaces here

src/datepicker/datepicker-i18n.ts Outdated
@@ -26,6 +28,12 @@ export abstract class NgbDatepickerI18n {
* With default calendar we use ISO 8601: 'month' is 1=January ... 12=December
*/
abstract getMonthFullName(month: number): string;

/**
* Returns the full month name to display in the date picker navigation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to update the comment for getDayAriaLabel

src/datepicker/datepicker-i18n.ts Outdated
@@ -34,7 +42,7 @@ export class NgbDatepickerI18nDefault extends NgbDatepickerI18n {
private _monthsShort: Array<string>;
private _monthsFull: Array<string>;

constructor(@Inject(LOCALE_ID) locale: string) {
constructor(@Inject(LOCALE_ID) private locale: string, private datePipe: DatePipe) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be _locale and _datePipe as they're private according to repo style guide...

demo/src/app/components/datepicker/demos/calendars/datepicker-calendars.ts Outdated
@@ -24,6 +24,10 @@ export class IslamicI18n extends NgbDatepickerI18n {
getMonthFullName(month: number) {
return this.getMonthShortName(month);
}

getDayAriaLabel(date: NgbDateStruct): string {
return new Date(date.year, date.month - 1, date.day).toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative maybe just `${date.day}-${date.month}-${date.year}`?

@maxokorokov
Copy link
Member

Update: I played with nvda, voice over and jaws 17. Ok, except for jaws
With jaws 17 I have navigation with arrow keys between days that doesn't work at all: Chrome, Win 7

@maxokorokov maxokorokov mentioned this pull request Apr 18, 2018
Closed
@maxokorokov maxokorokov added this to the 2.1.0 milestone Apr 18, 2018
maxokorokov
maxokorokov reviewed Apr 23, 2018
View reviewed changes
src/datepicker/datepicker-i18n.ts Outdated

getDayAriaLabel(date: NgbDateStruct): string {
const jsDate = new Date(date.year, date.month - 1, date.day);
return this.datePipe.transform(date, 'fullDate', null, this.locale);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you have a bug here date → jsDate

@maxokorokov
Copy link
Member

An update for JAWS:
JAWS 18 → everything works for me
JAWS 17:

  • navigation with arrow keys doesn't work without role=application
  • with role=application it says not in the table when trying to read a div with role=gridcell

maxokorokov
maxokorokov requested changes Apr 24, 2018
View reviewed changes
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, LGTM, but tests are still incomplete...

demo/src/app/components/datepicker/demos/i18n/datepicker-i18n.ts Outdated
@@ -33,6 +33,10 @@ export class CustomDatepickerI18n extends NgbDatepickerI18n {
getMonthFullName(month: number): string {
return this.getMonthShortName(month);
}

getDayAriaLabel(date: NgbDateStruct): string {
return new Date(date.year, date.month - 1, date.day).toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed this one → ${date.day}-${date.month}-${date.year}

demo/src/app/components/datepicker/datepicker.component.ts Outdated
@@ -33,13 +33,13 @@ import {DEMO_SNIPPETS} from './demos';
<ngbd-datepicker-adapter></ngbd-datepicker-adapter>
</ngbd-example-box>
<ngbd-example-box demoTitle="Internationalization of datepickers" [snippets]="snippets" component="datepicker" demo="i18n">
<ngbd-datepicker-i18n></ngbd-datepicker-i18n>
<ngbd-datepicker-i18n></ngbd-datepicker-i18n>
Copy link
Member

@maxokorokov maxokorokov Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you revert all changes in this file please, as they just break formatting?

src/datepicker/datepicker-tools.ts
@@ -120,6 +123,8 @@ export function buildMonth(calendar: NgbCalendar, date: NgbDate, state: Datepick
const newDate = new NgbDate(date.year, date.month, date.day);
const nextDate = calendar.getNext(newDate);

const ariaLabel = i18n.getDayAriaLabel(newDate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the previous review, could you please add a couple of tests to the detepicker-service.spec.ts that will test that labels on the days?

You would have caught unused jsDate bug the service if you had one...

@fbasso fbasso force-pushed the datepicker_aria branch 2 times, most recently from 67f2763 to c88ecf2 Compare April 24, 2018 12:27
@maxokorokov maxokorokov modified the milestones: 2.1.0, 2.0.0 Apr 24, 2018
@maxokorokov
Copy link
Member

@fbasso, clang-format is unhappy...

@maxokorokov maxokorokov merged commit d52059b into ng-bootstrap:master Apr 27, 2018
@benouat benouat mentioned this pull request Jun 1, 2018
Closed
@fbasso fbasso deleted the datepicker_aria branch June 5, 2018 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxokorokov maxokorokov maxokorokov approved these changes

Assignees

@maxokorokov maxokorokov

Labels
breaking change component: datepicker type: accessibility type: feature
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fbasso @maxokorokov