[New] Added phrase for aria-label for the selected day #905

erin-doyle · 2017-12-18T16:34:14Z

Added the setting of the CalendarDay aria-label with a dateIsSelected phrase when the day is the selected day.

erin-doyle · 2017-12-18T16:36:16Z

src/defaultPhrases.js

 const chooseAvailableStartDate = ({ date }) => `Choose ${date} as your check-in date. It's available.`;
-
-// eslint-disable-next-line camelcase


I removed these disable comments as these lines aren't failing that rule anymore

ljharb

LGTM overall

ljharb · 2017-12-18T16:37:23Z

src/components/CustomizableCalendarDay.jsx

@@ -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);


Can you think of any clean way to share this logic with the identical code in CalendarDay?

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.

ljharb · 2017-12-18T16:38:26Z

test/components/CalendarDay_spec.jsx

+        const selectedModifiers = new Set(['selected', 'selected-start', 'selected-end']);
+
+        selectedModifiers.forEach((selectedModifier) => {
+          const modifiers = new Set().add(selectedModifier);


new Set([selectedModifier])?

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.

fair, then it's fine to keep it :-)

ljharb · 2017-12-18T16:39:28Z

test/components/CalendarDay_spec.jsx

+        selectedModifiers.forEach((selectedModifier) => {
+          const modifiers = new Set().add(selectedModifier);
+
+          const wrapper = shallow(<CalendarDay


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)

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.

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.

I'll do a quick sweep and clean them up.

ljharb · 2017-12-18T16:39:38Z

test/components/CustomizableCalendarDay_spec.jsx

+        selectedModifiers.forEach((selectedModifier) => {
+          const modifiers = new Set().add(selectedModifier);
+
+          const wrapper = shallow(<CustomizableCalendarDay


coveralls · 2017-12-18T16:41:26Z

Coverage increased (+0.08%) to 85.57% when pulling cbafbbd on add-aria-label-selected-date into 92cdc04 on master.

erin-doyle · 2017-12-18T20:45:31Z

src/components/CalendarDay.jsx

+      hoveredSpan,
+      isOutsideRange,
+      ariaLabel,
+    } = getCalendarDaySettings(day, ariaLabelFormat, daySize, modifiers, phrases);


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.

i don't love it, but it's better than the duplication! :-D

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. ;)

coveralls · 2017-12-18T20:55:55Z

Coverage increased (+0.02%) to 85.505% when pulling ce272db on add-aria-label-selected-date into 92cdc04 on master.

erin-doyle · 2018-01-03T20:32:36Z

Rebase pushed.

coveralls · 2018-01-03T21:11:54Z

Coverage increased (+0.02%) to 86.266% when pulling 413bd51 on add-aria-label-selected-date into 9780545 on master.

erin-doyle · 2018-01-04T21:46:48Z

I just want to give this one a bump before I rebase and force @ljharb to approve it again 😉 .

majapw

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?

majapw · 2018-01-05T01:22:13Z

Also leaving a reminder for myself that we need to translate the selected phrase for airbnb. :P

ljharb · 2018-01-05T01:31:24Z

I think tiny well-tested utility methods is a good idea.

majapw · 2018-01-05T01:34:54Z

That way both CalendarDay components could call the methods explicitly but the logic of what they did would be in one place?

ljharb · 2018-01-05T05:50:56Z

Yep! Exactly. One source of truth means bugs are either less likely or more likely to be caught :-)

erin-doyle · 2018-01-05T13:16:29Z

So are you saying you'd like something like:

getDaySizeStyles()
useDefaultCursorOnDay()
isDaySelected()
isDayHovered()
isDayOutsideRange()
getDayAriaLabel()

and would these each be their own file under utils?

I can go ahead and make those changes (in a separate PR preferably 😉).

erin-doyle · 2018-01-05T13:17:36Z

I'm going to go ahead and rebase this now.

coveralls · 2018-01-05T13:35:06Z

Coverage increased (+0.02%) to 86.275% when pulling a178447 on add-aria-label-selected-date into 1331539 on master.

majapw

@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.

erin-doyle · 2018-01-05T17:55:38Z

Ok @majapw sounds good and thanks!

erin-doyle · 2018-01-09T14:00:33Z

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.

coveralls · 2018-01-09T14:19:51Z

Coverage increased (+0.02%) to 86.275% when pulling c997977 on add-aria-label-selected-date into 048612a on master.

erin-doyle commented Dec 18, 2017

View reviewed changes

ljharb reviewed Dec 18, 2017

View reviewed changes

ljharb added the semver-minor: new stuff Any feature or API addition. label Dec 18, 2017

erin-doyle commented Dec 18, 2017

View reviewed changes

ljharb previously approved these changes Dec 18, 2017

View reviewed changes

ljharb requested a review from majapw December 18, 2017 22:56

erin-doyle dismissed ljharb’s stale review via 413bd51 January 3, 2018 20:32

erin-doyle force-pushed the add-aria-label-selected-date branch from ce272db to 413bd51 Compare January 3, 2018 20:32

ljharb previously approved these changes Jan 3, 2018

View reviewed changes

majapw previously approved these changes Jan 5, 2018

View reviewed changes

erin-doyle dismissed stale reviews from majapw and ljharb via a178447 January 5, 2018 13:19

erin-doyle force-pushed the add-aria-label-selected-date branch from 413bd51 to a178447 Compare January 5, 2018 13:19

majapw previously approved these changes Jan 5, 2018

View reviewed changes

Erin Doyle added 2 commits January 9, 2018 09:03

Added the setting of the CalendarDay aria-label with a dateIsSelected…

e6cf29e

… phrase when the day is the selected day.

Added a getCalendarDaySettings utility function that can be shared by…

de49570

… CalendarDay and CustomizableCalendarDay.

Tests clean up

c997977

erin-doyle dismissed majapw’s stale review via c997977 January 9, 2018 14:04

erin-doyle force-pushed the add-aria-label-selected-date branch from a178447 to c997977 Compare January 9, 2018 14:04

ljharb approved these changes Jan 9, 2018

View reviewed changes

ljharb merged commit aa1d770 into master Jan 9, 2018

ljharb deleted the add-aria-label-selected-date branch January 9, 2018 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] Added phrase for aria-label for the selected day #905

[New] Added phrase for aria-label for the selected day #905

erin-doyle commented Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

ljharb left a comment

ljharb Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

ljharb Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

coveralls commented Dec 18, 2017

erin-doyle Dec 18, 2017

ljharb Dec 18, 2017

erin-doyle Dec 18, 2017

coveralls commented Dec 18, 2017

erin-doyle commented Jan 3, 2018

coveralls commented Jan 3, 2018

erin-doyle commented Jan 4, 2018

majapw left a comment

majapw commented Jan 5, 2018

ljharb commented Jan 5, 2018

majapw commented Jan 5, 2018

ljharb commented Jan 5, 2018

erin-doyle commented Jan 5, 2018

erin-doyle commented Jan 5, 2018

coveralls commented Jan 5, 2018

majapw left a comment

erin-doyle commented Jan 5, 2018

erin-doyle commented Jan 9, 2018

coveralls commented Jan 9, 2018

		const chooseAvailableStartDate = ({ date }) => `Choose ${date} as your check-in date. It's available.`;

		// eslint-disable-next-line camelcase

[New] Added phrase for aria-label for the selected day #905

[New] Added phrase for aria-label for the selected day #905

Conversation

erin-doyle commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2017

erin-doyle commented Jan 3, 2018

coveralls commented Jan 3, 2018

erin-doyle commented Jan 4, 2018

majapw left a comment

Choose a reason for hiding this comment

majapw commented Jan 5, 2018

ljharb commented Jan 5, 2018

majapw commented Jan 5, 2018

ljharb commented Jan 5, 2018

erin-doyle commented Jan 5, 2018

erin-doyle commented Jan 5, 2018

coveralls commented Jan 5, 2018

majapw left a comment

Choose a reason for hiding this comment

erin-doyle commented Jan 5, 2018

erin-doyle commented Jan 9, 2018

coveralls commented Jan 9, 2018