-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Clarify VoiceOver text for dates selected as start-date and end-date #1501
Clarify VoiceOver text for dates selected as start-date and end-date #1501
Conversation
src/utils/getCalendarDaySettings.js
Outdated
@@ -36,7 +38,13 @@ export default function getCalendarDaySettings(day, ariaLabelFormat, daySize, mo | |||
|
|||
let ariaLabel = getPhrase(chooseAvailableDate, formattedDate); | |||
if (selected) { | |||
ariaLabel = getPhrase(dateIsSelected, formattedDate); | |||
if (modifiers.has('selected-start')) { |
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.
Should I add a null check here for dateIsSelectedAsCheckin
and dateIsSelectedAsCheckout
, so that it doesn't throw an error if someone using react-dates is overriding the default phrases and hasn't added these two phrases?
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.
That might be a good mitigating factor.
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.
Looks great! Would love to use startdate/enddate instead of checkin/checkout but other than that looks good!
src/defaultPhrases.js
Outdated
@@ -128,6 +134,8 @@ export const SingleDatePickerPhrases = { | |||
chooseAvailableDate, | |||
dateIsUnavailable, | |||
dateIsSelected, | |||
dateIsSelectedAsCheckin, |
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.
I don't think the SDP needs these as it only has one date
src/defaultPhrases.js
Outdated
@@ -30,6 +30,8 @@ const chooseAvailableEndDate = ({ date }) => `Choose ${date} as your check-out d | |||
const chooseAvailableDate = ({ date }) => date; | |||
const dateIsUnavailable = ({ date }) => `Not available. ${date}`; | |||
const dateIsSelected = ({ date }) => `Selected. ${date}`; | |||
const dateIsSelectedAsCheckin = ({ date }) => `Selected for check-in. ${date}`; |
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.
Can we s/checkin/startdate and s/checkout/enddate for consistency with this library? We can use checkin/checkout at airbnb but it'd be good to keep the nomenclature consistent within this library.
src/utils/getCalendarDaySettings.js
Outdated
@@ -36,7 +38,13 @@ export default function getCalendarDaySettings(day, ariaLabelFormat, daySize, mo | |||
|
|||
let ariaLabel = getPhrase(chooseAvailableDate, formattedDate); | |||
if (selected) { | |||
ariaLabel = getPhrase(dateIsSelected, formattedDate); | |||
if (modifiers.has('selected-start')) { |
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.
That might be a good mitigating factor.
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.
Lookin' good!
This PR adds clarity to the VO text for dates selected as check-in and check-out. Previously, the VO text for all selected dates was "Selected. ${date}", and I have updated it so that if the date is selected for check-in it is "Selected for check-in. ${date}" and if it is selected for check-out it is "Selected for check-out. ${date}".
jira ticket: https://jira.airbnb.biz/browse/PRODUCT-56423