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.

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

feat(datepicker): indicate selected day in aria label #242

Merged
merged 1 commit into from Aug 1, 2022

Conversation

d-kostov-dev
Copy link

@d-kostov-dev d-kostov-dev commented Jul 11, 2022

fixes VPAT-3637

PR Type
Feature - Accessibility

What is the current behavior?
The currently selected date is not indicated to screen reader users.

What is the new behavior?
The aria-label of the day is now showing the word "Selected".

Does this PR introduce a breaking change?
No.

@vmwclabot
Copy link

@d-kostov-dev, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 11, 2022

👋 @kevinbuhmann,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, checkout out a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal #clarity-support Slack channel

Thank you,

🤖 Clarity Release Bot

@kevinbuhmann kevinbuhmann changed the title Day component will now show if a day is selected in the aria-label. feat(datepicker): indicate selected day in aria label Jul 11, 2022
@kevinbuhmann
Copy link
Member

I force pushed this branch to get the PR building for a11y review and to reword the commit message to follow our conventions.

@vmwclabot
Copy link

@d-kostov-dev, your company's legal contact has approved your signed contributor license agreement. It will also be reviewed by VMware, but the merge can proceed.

@ashleyryan
Copy link

Thanks for your contribution @d-kostov-dev. We're discussing this fix with our Accessibility contact (who will most likely be speaking to yours) before we approve this.

@ashleyryan
Copy link

@d-kostov-dev Here is the recommendation from our A11Y expert:

Because aria-selected is not widely supported in either td or button for screen readers we recommend using aria-label="[full date] - Selected" instead. Additionally aria-current="date" should also be provided to indicate the current date as Clarity's date picker does show in visually in bold what the current date is when it is not selected.

They included a picture too:
image

Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

See comment. Also, please fix the build errors (including lint and test errors).

projects/angular/src/utils/i18n/common-strings.default.ts Outdated Show resolved Hide resolved
Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

See comment.

Also, please squash the commits into one "feat(datepicker): indicate selected day in aria-label" commit because this should be a single, atomic change.

projects/angular/src/forms/datepicker/day.ts Outdated Show resolved Hide resolved
Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

Please also run npm run build && npm run public-api:update to update the public API file.

Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

Also, please add a test for this change. You can use this if you want (after the "sets the correct value for the aria-label attribute" test):

it('sets the correct value for the aria-label attribute when the date is selected', () => {
  const dayBtn: HTMLButtonElement = context.clarityElement.children[0];
  const dvm: DayViewModel = context.clarityDirective.dayView;

  context.testComponent.dayView.isSelected = true;

  context.detectChanges();
  expect(dayBtn.attributes['aria-label'].value).toEqual(`${dvm.dayModel.toDateString()} - SELECTED`);
});

@vmwclabot
Copy link

@d-kostov-dev, VMware has approved your signed contributor license agreement.

Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

Please squash the commits and keep the first commit message. These commits are not compatible with our release process.

Also, please rebase and run npm run build && npm run public-api:update to fix the build error.

@kevinbuhmann
Copy link
Member

kevinbuhmann commented Jul 28, 2022

Sorry if you are still working on the rebase, but I see the merge commit you just pushed. We don't want a merge commit. There should be just one commit without any of the aria-current stuff. I have already merged that feature via a different PR.

fixes VPAT-3637

PR Type
Feature - Accessibility

What is the current behavior?
The currently selected date is not indicated to screen reader users.

What is the new behavior?
The aria-label of the day is now showing the word "Selected".

Does this PR introduce a breaking change?
No.
@kevinbuhmann
Copy link
Member

I went ahead and rebased for you to remove the aria-current changes from this commit. When the build passes, I will merge.

@kevinbuhmann kevinbuhmann merged commit 76a832d into vmware-clarity:main Aug 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

🎉 This PR is included in version 13.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants