Separates out focusedInput from DOM Focus and focuses on the calendar for withPortal implementations #410
Conversation
majapw
added
the
semver-patch: fixes/refactors/etc
src/components/DateRangePicker.jsx
Outdated
| const { onFocusChange, withPortal, withFullScreenPortal } = this.props; | ||
|
|
||
| if (focusedInput) { | ||
| const moveFocusToDayPicker = withPortal || withFullScreenPortal; |
There was a problem hiding this comment.
Would it make more sense to target based on touch instead of portal presence? The mobile vertical scrollable date picker doesn't use these portal props, but it also requires daypicker focus.
There was a problem hiding this comment.
hmm, well my thought is that it only makes sense to do this when the input is not visible... but maybe on touch devices makes sense too. Is the input going to be visible at the top of the VERTICAL_SCROLLABLE DayPicker? I forget what the designs look like
There was a problem hiding this comment.
The input will always be visible with VERTICAL_SCROLLABLE, and the focus should stick back to the day picker after tapping an input.
There was a problem hiding this comment.
Okay, let's have this occur always for portals and also always on touch devices.
| // i18n | ||
| phrases: DateRangePickerInputPhrases, | ||
| }; | ||
|
|
||
| export default class DateRangePickerInputWithHandlers extends React.Component { | ||
| export default class DateRangePickerInputController extends React.Component { |
There was a problem hiding this comment.
💯 This has thrown a wrench in many of my greps
majapw
force-pushed
the
maja-fix-done-bottom-bar-on-ios
branch
2 times, most recently
from
f164861
to
93fde60
Compare
|
@moonboots PTAL |
| @@ -19,6 +19,8 @@ const propTypes = forbidExtraProps({ | |||
| onFocus: PropTypes.func, | |||
| onKeyDownShiftTab: PropTypes.func, | |||
| onKeyDownTab: PropTypes.func, | |||
|
|
|||
| isFocused: PropTypes.bool, // describes actual DOM focus | |||
There was a problem hiding this comment.
A little confused about the distinction between this bool and focused by name alone. Seems like it'd be better as something like focusInputDOM or something that makes it more explicit.
There was a problem hiding this comment.
I wanted parity with focusing the DayPicker as well.
src/components/DateRangePicker.jsx
Outdated
| this.setState({ | ||
| isDateRangePickerInputFocused: false, | ||
| isDayPickerFocused: false, | ||
| showKeyboardShortcuts: false, |
There was a problem hiding this comment.
this doesn't exist yet does it? Isn't that in the a11y PR?
src/components/DayPicker.jsx
Outdated
| className="DayPicker__focus-region" | ||
| ref={(ref) => { this.container = ref; }} | ||
| role="region" | ||
| tabIndex={0} |
There was a problem hiding this comment.
is this something you specifically want users to be able to tab to or just to focus? Seems like something that should be more for focusing purposes, in which case you'd want a tabindex of -1
majapw
force-pushed
the
maja-fix-done-bottom-bar-on-ios
branch
from
93fde60
to
03d5326
Compare
… for withPortal implementations
majapw
force-pushed
the
maja-fix-done-bottom-bar-on-ios
branch
from
03d5326
to
a4b328f
Compare
This, as far as I can tell, should be a fix for #246.
I'm going to favor this PR over #409 because it paves the way better for #301.
@airbnb/webinfra @moonboots PTAL