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

Customize DayPickerKeyboardShortcuts with renderKeyboardShortcutsButton #1576

Merged
merged 11 commits into from Mar 12, 2019

Conversation

Projects
None yet
5 participants
@kypan
Copy link

kypan commented Mar 8, 2019

Introducing a functional prop renderKeyboardShortcutsButton to render a custom styled button.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 8, 2019

Coverage Status

Coverage remained the same at 84.486% when pulling 98fe176 on kypan:kevin--customize-day-picker-keyboard-shortcuts-color into 64c9857 on airbnb:master.

@monokrome
Copy link
Contributor

monokrome left a comment

Looks good, but it's hard to test. Could you add a variant w/ this enabled to storybook?

topLeft && styles.DayPickerKeyboardShortcuts_showSpan__topLeft,
)}
>
{ renderKeyboardShortcutsButton

This comment has been minimized.

@monokrome

monokrome Mar 8, 2019

Contributor

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

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Member

(in jsx in general)

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Member

also, jsx curlies should have no bounding spaces inside them

topLeft && styles.DayPickerKeyboardShortcuts_show__topLeft,
)}
type="button"
aria-label={toggleButtonText}

This comment has been minimized.

@kypan

kypan Mar 8, 2019

Author

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

This comment has been minimized.

@monokrome

monokrome Mar 8, 2019

Contributor

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

This comment has been minimized.

@kypan

kypan Mar 8, 2019

Author

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

This comment has been minimized.

@monokrome

monokrome Mar 8, 2019

Contributor

Cool! Thanks so much for explaining to me 😺

topLeft && styles.DayPickerKeyboardShortcuts_showSpan__topLeft,
)}
>
{ renderKeyboardShortcutsButton

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Member

also, jsx curlies should have no bounding spaces inside them

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

This comment has been minimized.

@ljharb

ljharb Mar 8, 2019

Member

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?

This comment has been minimized.

@kypan

kypan Mar 8, 2019

Author

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

This comment has been minimized.

@ljharb

ljharb Mar 9, 2019

Member

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

This comment has been minimized.

@kypan

kypan Mar 11, 2019

Author

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

@ljharb
Copy link
Member

ljharb left a comment

It’d be good to also add test coverage for DayPicker and DayPickerRangeController - ie, just ensuring that the props are passed down.

Show resolved Hide resolved src/components/DayPickerKeyboardShortcuts.jsx Outdated
@@ -68,6 +68,49 @@ const TestCustomInfoPanel = () => (
</div>
);

function renderKeyboardShortcutsButton(buttonProps) {
const { ref, onClick, ariaLabel } = buttonProps || {};

This comment has been minimized.

@ljharb

ljharb Mar 11, 2019

Member

Why would buttonProps need to be optional here?

If it is, it should be defaulted in the signature, no?

This comment has been minimized.

@kypan

kypan Mar 11, 2019

Author

@ljharb I think when I wrote this I copied code from monorail. I intended to make buttonProps optional to avoid future implementations breaking, and so I used the defaulting pattern I saw in monorail. But if it's cool with the react-dates squad I can just make it required

This comment has been minimized.

@ljharb

ljharb Mar 11, 2019

Member

Seems better to require everything until it needs to be optional.

This comment has been minimized.

@kypan

kypan Mar 11, 2019

Author

Cool, I made it required

@monokrome
Copy link
Contributor

monokrome left a comment

Looks good after Jordan's requests are implemented 😺

@kypan

This comment has been minimized.

Copy link
Author

kypan commented Mar 11, 2019

@ljharb Added specs for DayPicker and DayPickerRangeController

Show resolved Hide resolved stories/DayPickerRangeController.js Outdated
Show resolved Hide resolved stories/DayPickerRangeController.js Outdated
Show resolved Hide resolved stories/DayPickerRangeController.js Outdated
Kevin Pan
@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Mar 12, 2019

Sick! This looks awesome. Thank you for the thorough reviews @monokrome and @ljharb

@majapw majapw merged commit cffd919 into airbnb:master Mar 12, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.486%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.