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

Customize DayPickerKeyboardShortcuts with renderKeyboardShortcutsButton #1576

Merged
merged 11 commits into from
Mar 12, 2019
Merged

Customize DayPickerKeyboardShortcuts with renderKeyboardShortcutsButton #1576

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4ad7be8
adding renderKeyboardShortcutsButton functional prop
Mar 7, 2019
1b8465d
fixing renderKeyboardShortcutsButton functional prop
Mar 7, 2019
111494f
small null check fix
Mar 7, 2019
4e94f84
typo and other fixes
Mar 8, 2019
22d6a4b
adding props at render to button
Mar 8, 2019
92052b5
replace deleted renderCalendarInfo
Mar 8, 2019
8b62e7b
removing need to clone button
Mar 8, 2019
ce25884
PR fixes: adding params to renderKeyboardShortcutsButton; updating specs
Mar 9, 2019
ba397af
adding storybook variation
Mar 9, 2019
116aacf
PR fixes: syntax issues, adding specs, misc fixes
Mar 11, 2019
98fe176
linting issues
Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/DayPicker.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const propTypes = forbidExtraProps({
transitionDuration: nonNegativeInteger,
verticalBorderSpacing: nonNegativeInteger,
horizontalMonthPadding: nonNegativeInteger,
renderKeyboardShortcutsButton: PropTypes.func,

// navigation props
disablePrev: PropTypes.bool,
Expand Down Expand Up @@ -131,6 +132,7 @@ export const defaultProps = {
transitionDuration: undefined,
verticalBorderSpacing: undefined,
horizontalMonthPadding: 13,
renderKeyboardShortcutsButton: undefined,

// navigation props
disablePrev: false,
Expand Down Expand Up @@ -930,6 +932,7 @@ class DayPicker extends React.PureComponent {
renderDayContents,
renderCalendarInfo,
renderMonthElement,
renderKeyboardShortcutsButton,
calendarInfoPosition,
hideKeyboardShortcutsPanel,
onOutsideClick,
Expand Down Expand Up @@ -1121,6 +1124,7 @@ class DayPicker extends React.PureComponent {
openKeyboardShortcutsPanel={this.openKeyboardShortcutsPanel}
closeKeyboardShortcutsPanel={this.closeKeyboardShortcutsPanel}
phrases={phrases}
renderKeyboardShortcutsButton={renderKeyboardShortcutsButton}
/>
)}
</div>
Expand Down
58 changes: 32 additions & 26 deletions src/components/DayPickerKeyboardShortcuts.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const propTypes = forbidExtraProps({
openKeyboardShortcutsPanel: PropTypes.func,
closeKeyboardShortcutsPanel: PropTypes.func,
phrases: PropTypes.shape(getPhrasePropTypes(DayPickerKeyboardShortcutsPhrases)),
renderKeyboardShortcutsButton: PropTypes.func,
});

const defaultProps = {
Expand All @@ -31,6 +32,7 @@ const defaultProps = {
openKeyboardShortcutsPanel() {},
closeKeyboardShortcutsPanel() {},
phrases: DayPickerKeyboardShortcutsPhrases,
renderKeyboardShortcutsButton: undefined,
};

function getKeyboardShortcuts(phrases) {
Expand Down Expand Up @@ -164,6 +166,7 @@ class DayPickerKeyboardShortcuts extends React.PureComponent {
closeKeyboardShortcutsPanel,
styles,
phrases,
renderKeyboardShortcutsButton,
} = this.props;

const toggleButtonText = showKeyboardShortcutsPanel
Expand All @@ -176,33 +179,36 @@ class DayPickerKeyboardShortcuts extends React.PureComponent {

return (
<div>
<button
ref={this.setShowKeyboardShortcutsButtonRef}
{...css(
styles.DayPickerKeyboardShortcuts_buttonReset,
styles.DayPickerKeyboardShortcuts_show,
bottomRight && styles.DayPickerKeyboardShortcuts_show__bottomRight,
topRight && styles.DayPickerKeyboardShortcuts_show__topRight,
topLeft && styles.DayPickerKeyboardShortcuts_show__topLeft,
)}
type="button"
aria-label={toggleButtonText}
onClick={this.onShowKeyboardShortcutsButtonClick}
onMouseUp={(e) => {
e.currentTarget.blur();
}}
>
<span
{...css(
styles.DayPickerKeyboardShortcuts_showSpan,
bottomRight && styles.DayPickerKeyboardShortcuts_showSpan__bottomRight,
topRight && styles.DayPickerKeyboardShortcuts_showSpan__topRight,
topLeft && styles.DayPickerKeyboardShortcuts_showSpan__topLeft,
)}
>
{ renderKeyboardShortcutsButton
Copy link
Contributor

@monokrome monokrome Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is a nit, but I think that the code style in react-dates is to avoid ternaries by creating two conditions instead?

 { renderKeyboardShortcutsButton && .... }
 { renderKeyboardShortcutsButton || .... }

Copy link
Member

Choose a reason for hiding this comment

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

(in jsx in general)

Copy link
Member

Choose a reason for hiding this comment

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

also, jsx curlies should have no bounding spaces inside them

? renderKeyboardShortcutsButton() : (
<button
ref={this.setShowKeyboardShortcutsButtonRef}
{...css(
styles.DayPickerKeyboardShortcuts_buttonReset,
styles.DayPickerKeyboardShortcuts_show,
bottomRight && styles.DayPickerKeyboardShortcuts_show__bottomRight,
topRight && styles.DayPickerKeyboardShortcuts_show__topRight,
topLeft && styles.DayPickerKeyboardShortcuts_show__topLeft,
)}
type="button"
aria-label={toggleButtonText}
Copy link
Author

@kypan kypan Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In order to pass in aria-label from custom button, I might have to change this to this.getToggleButtonText() and bind to this scope since it relies on state change in DayPicker.jsx

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused, what is being referred to as "custom button" here? Seems like aria-label would work here unless I misunderstand?

Copy link
Author

Choose a reason for hiding this comment

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

@monokrome toggleButtonText here relies on showKeyboardShortcuts which is a state variable set in the parent DayPicker.jsx. At first, I didn't think there was a way for me to pass in a similar variable through renderKeyboardsShortcutsButton that can read the state of DayPicker.jsx unless I bound it somehow.

But actually though, I just realized that I can pass in toggleButtonText as a param into the renderKeyboardShortcutsButton() here and drill that back into the render logic. Turns out I don't need to change any logic here. Hope that made sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Thanks so much for explaining to me 😺

onClick={this.onShowKeyboardShortcutsButtonClick}
onMouseUp={(e) => {
e.currentTarget.blur();
}}
>
<span
{...css(
styles.DayPickerKeyboardShortcuts_showSpan,
bottomRight && styles.DayPickerKeyboardShortcuts_showSpan__bottomRight,
topRight && styles.DayPickerKeyboardShortcuts_showSpan__topRight,
topLeft && styles.DayPickerKeyboardShortcuts_showSpan__topLeft,
)}
>
?
</span>
</button>
</span>
</button>
)}
kypan marked this conversation as resolved.
Show resolved Hide resolved

{showKeyboardShortcutsPanel && (
<div
Expand Down
4 changes: 4 additions & 0 deletions src/components/DayPickerRangeController.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const propTypes = forbidExtraProps({
renderCalendarDay: PropTypes.func,
renderDayContents: PropTypes.func,
renderCalendarInfo: PropTypes.func,
renderKeyboardShortcutsButton: PropTypes.func,
calendarInfoPosition: CalendarInfoPositionShape,
firstDayOfWeek: DayOfWeekShape,
verticalHeight: nonNegativeInteger,
Expand Down Expand Up @@ -149,6 +150,7 @@ const defaultProps = {
renderDayContents: null,
renderCalendarInfo: null,
renderMonthElement: null,
renderKeyboardShortcutsButton: undefined,
calendarInfoPosition: INFO_POSITION_BOTTOM,
firstDayOfWeek: null,
verticalHeight: null,
Expand Down Expand Up @@ -1234,6 +1236,7 @@ export default class DayPickerRangeController extends React.PureComponent {
withPortal,
enableOutsideDays,
firstDayOfWeek,
renderKeyboardShortcutsButton,
hideKeyboardShortcutsPanel,
daySize,
focusedInput,
Expand Down Expand Up @@ -1297,6 +1300,7 @@ export default class DayPickerRangeController extends React.PureComponent {
renderDayContents={renderDayContents}
renderCalendarInfo={renderCalendarInfo}
renderMonthElement={renderMonthElement}
renderKeyboardShortcutsButton={renderKeyboardShortcutsButton}
calendarInfoPosition={calendarInfoPosition}
firstDayOfWeek={firstDayOfWeek}
hideKeyboardShortcutsPanel={hideKeyboardShortcutsPanel}
Expand Down
9 changes: 9 additions & 0 deletions test/components/DayPickerKeyboardShortcuts_spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ describe('DayPickerKeyboardShortcuts', () => {
expect(mockEvent.currentTarget.blur.callCount).to.equal(1);
});
});

describe('renderKeyboardShortcutsButton', () => {
it('renders the provided button', () => {
const props = { renderKeyboardShortcutsButton: () => (<button type="button">Success!</button>) };
const wrapper = shallow(<DayPickerKeyboardShortcuts {...props} />).dive();
const buttonWrapper = wrapper.children().find('button');
Copy link
Member

Choose a reason for hiding this comment

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

possibly a more robust test would be to render a custom component, and expect(wrapper.find(Custom)).to.have.lengthOf(1)), rather than duplicating the text and element name here?

Copy link
Author

@kypan kypan Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The use case for renderKeyboardShortcutsButton is to literally send in a <button> though, should I still create a spec for a React Component? @ljharb

Copy link
Member

Choose a reason for hiding this comment

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

@kypan i mean, the purpose is to ensure that whatever renderKeyboardShortcutsButton returns is what's rendered; the only way to guarantee that is to use an unforgeable sentinel value - which would thus be a custom component.

Copy link
Author

Choose a reason for hiding this comment

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

@ljharb That definitely makes sense. I've updated it. Thanks!

expect(buttonWrapper.text()).to.equal('Success!');
});
});
});

describe('#DayPickerKeyboardShortcuts_panel', () => {
Expand Down