-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[New] Added phrase for aria-label for the selected day #905
Conversation
const chooseAvailableStartDate = ({ date }) => `Choose ${date} as your check-in date. It's available.`; | ||
|
||
// eslint-disable-next-line camelcase |
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 removed these disable comments as these lines aren't failing that rule anymore
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.
Thanks!
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.
LGTM overall
@@ -207,6 +202,15 @@ class CustomizableCalendarDay extends React.Component { | |||
|
|||
const isOutsideRange = modifiers.has('blocked-out-of-range'); | |||
|
|||
const formattedDate = { date: day.format(ariaLabelFormat) }; | |||
|
|||
let ariaLabel = getPhrase(chooseAvailableDate, formattedDate); |
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 you think of any clean way to share this logic with the identical code in CalendarDay?
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.
There's a lot more than just this duplicated between CalendarDay and CustomizableCalendarDay now. I could put together a utility function somewhere though and call it from each to get the ariaLabel value.
test/components/CalendarDay_spec.jsx
Outdated
const selectedModifiers = new Set(['selected', 'selected-start', 'selected-end']); | ||
|
||
selectedModifiers.forEach((selectedModifier) => { | ||
const modifiers = new Set().add(selectedModifier); |
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.
new Set([selectedModifier])
?
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.
Yep that works fine and I can change it, the use of new Set().add(<whatever>);
is used heavily in this file so I was following in kind.
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.
fair, then it's fine to keep it :-)
test/components/CalendarDay_spec.jsx
Outdated
selectedModifiers.forEach((selectedModifier) => { | ||
const modifiers = new Set().add(selectedModifier); | ||
|
||
const wrapper = shallow(<CalendarDay |
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.
Not a huge deal, but the (
should end a line here, and the )
should begin once - the rest of the indentation will follow that. (You may need to double-wrap the jsx in parens to satisfy the linter)
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.
Same as above comment, this is the style that is used throughout this entire file so I was just going with the flow, but I can certainly change it 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.
the other one is fine, but this one goes against (the spirit, at least, of) the airbnb styleguide, so we should clean this pattern 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.
I'll do a quick sweep and clean them up.
selectedModifiers.forEach((selectedModifier) => { | ||
const modifiers = new Set().add(selectedModifier); | ||
|
||
const wrapper = shallow(<CustomizableCalendarDay |
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.
also here
hoveredSpan, | ||
isOutsideRange, | ||
ariaLabel, | ||
} = getCalendarDaySettings(day, ariaLabelFormat, daySize, modifiers, 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.
Not sure if you'll hate this idea or not but it was my stab at consolidating some of the duplicate code between CalenderDay and CustomizableCalendarDay in the render() function.
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 love it, but it's better than the duplication! :-D
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.
Ha! Agreed! I feel like there must be a design pattern that will solve for all the duplication but it felt out of scope of this change. I think this sort-of-icky utility function will do in the meantime. ;)
ce272db
to
413bd51
Compare
Rebase pushed. |
I just want to give this one a bump before I rebase and force @ljharb to approve it again 😉 . |
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 seems reasonable to me as well. :/
A lot of this duplication is in fact I feel par for the course in this library, but it makes sense to isolate.
Do you think it might be better to have tiny utility methods for everything? or just consolidate in the one method?
Also leaving a reminder for myself that we need to translate the selected phrase for airbnb. :P |
I think tiny well-tested utility methods is a good idea. |
That way both CalendarDay components could call the methods explicitly but the logic of what they did would be in one place? |
Yep! Exactly. One source of truth means bugs are either less likely or more likely to be caught :-) |
So are you saying you'd like something like:
and would these each be their own file under utils? I can go ahead and make those changes (in a separate PR preferably 😉). |
I'm going to go ahead and rebase this now. |
413bd51
to
a178447
Compare
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.
@erin-doyle yeah that was the general idea! But can def be done in a follow-up PR. :) This looks good.
I'll do a release today with our current queue of changes.
Ok @majapw sounds good and thanks! |
Oh jeez, this still needs to be merged but now I need to rebase again. 😿 I'm going to do the rebase now and I'm sorry but I'm guessing it's going to require one of you approves again. Once that's done I'll just merge it. |
… phrase when the day is the selected day.
… CalendarDay and CustomizableCalendarDay.
a178447
to
c997977
Compare
Added the setting of the CalendarDay aria-label with a dateIsSelected phrase when the day is the selected day.