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

Update the SDP and DRP to be compatible with react-with-direction #1482

Merged
merged 1 commit into from
Dec 10, 2018

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Dec 6, 2018

This change prevents automatic flipping on styles that are manually handled via the isRTL prop.

to: @airbnb/ui-frameworks @ljharb @yzimet

@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Dec 6, 2018
@majapw majapw force-pushed the maja-compat-react-with-direction branch from 2229c31 to 995f983 Compare December 6, 2018 01:12
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Instead of an isRTL prop, can we not use withDirection?

src/utils/noflip.js Outdated Show resolved Hide resolved
@yzimet
Copy link
Contributor

yzimet commented Dec 6, 2018

@ljharb We considered introducing withDirection as a dependency, but that would be a breaking change from the existing usage of the isRTL prop. We could still go that route in the future.

This PR hopes to prevent the collision between the manual flipping and the automatic flipping in the event that these components are wrapped in a DirectionProvider.

Copy link
Contributor

@yzimet yzimet left a comment

Choose a reason for hiding this comment

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

Thanks so much, @majapw !

src/components/DateRangePicker.jsx Outdated Show resolved Hide resolved
src/components/DateRangePicker.jsx Outdated Show resolved Hide resolved
src/components/DateRangePicker.jsx Outdated Show resolved Hide resolved
src/components/DateRangePicker.jsx Outdated Show resolved Hide resolved
src/components/DayPickerKeyboardShortcuts.jsx Outdated Show resolved Hide resolved
This change prevents automatic flipping on styles that are manually handled via the isRTL prop.

Co-Authored-By: majapw <majapw@gmail.com>
@majapw majapw force-pushed the maja-compat-react-with-direction branch from 540642a to f4535de Compare December 7, 2018 00:40
@majapw
Copy link
Collaborator Author

majapw commented Dec 7, 2018

@ljharb can you take another look?

@@ -67,12 +67,12 @@ export default withStyles(({ reactDates: { color } }) => ({
KeyboardShortcutRow_keyContainer: {
display: 'inline-block',
whiteSpace: 'nowrap',
textAlign: 'right',
marginRight: 6,
textAlign: 'right', // is not handled by isRTL
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean these should have a todo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think once we got to the keyboard shortcuts, we realized that this panel has just never been considered for RTL. So maybe? I think that is an open question.

@coveralls
Copy link

coveralls commented Dec 8, 2018

Coverage Status

Coverage increased (+0.02%) to 85.12% when pulling 995f983 on maja-compat-react-with-direction into 37f8aba on master.

@majapw majapw merged commit 819dafe into master Dec 10, 2018
@majapw majapw deleted the maja-compat-react-with-direction branch December 10, 2018 00:32
@yzimet
Copy link
Contributor

yzimet commented Dec 10, 2018

hmm @majapw it seems like we missed something, looking at the example in storybook (ref) the calendar is not visible:
image

@majapw
Copy link
Collaborator Author

majapw commented Dec 12, 2018

what the shit no but actually we looked at this, didn't we?

yzimet added a commit to yzimet/react-dates that referenced this pull request Dec 12, 2018
In react-dates#1482 we added compatibility with react-with-direction. In the process we
created a helper utility `noflip` to make it so that react-with-direction
wouldn't interfere with the preexisting `isRTL` flag. This PR fixes the noflip
behavior for Numbers, and adds unit tests.
monokrome pushed a commit that referenced this pull request Jan 3, 2019
In #1482 we added compatibility with react-with-direction. In the process we
created a helper utility `noflip` to make it so that react-with-direction
wouldn't interfere with the preexisting `isRTL` flag. This PR fixes the noflip
behavior for Numbers, and adds unit tests.
zeljkoX added a commit to pricemethod/react-dates that referenced this pull request Feb 7, 2019
* [a11y] Move triangular border styles to :before pseudo element, fixing lack of visible focus in Firefox and IE

* [a11y] Remove space/enter onKeyDown handling for open/close keyboard shortcuts panel

* Disable navigation controllers based on min and max date

* Add stories

* Early return if not a moment object

* Tests for block navigation

* Use disabled colors in navigation

* Rename to disablePrev/Next so the default is falsy

* disable navigation onKeyUp

* aria-disabled in navigation button

* Rename to shouldDisableMonthNavigation

* Init disablePrev and disableNext in component state

* Fix indentation

* Fallback to undefined in aria-disabled when not disabled

Co-Authored-By: pedroabreu <pmig.abreu@gmail.com>

* pre-populate state depending on initial props

* disable onMouseUp event by disable prop

* Update the SDP and DRP to be compatible with react-with-direction

This change prevents automatic flipping on styles that are manually handled via the isRTL prop.

Co-Authored-By: majapw <majapw@gmail.com>

* Version 18.3.0

* [RTL] Fix the SDP and DRP noflip util function

In react-dates#1482 we added compatibility with react-with-direction. In the process we
created a helper utility `noflip` to make it so that react-with-direction
wouldn't interfere with the preexisting `isRTL` flag. This PR fixes the noflip
behavior for Numbers, and adds unit tests.

* Version 18.3.1

* Add bug report issue template

* Add `openDirection` prop to README

* Clarify VoiceOver text for dates selected as check-in and check-out

* fixed tests

* addressed comments

* Version 18.4.0

* Add children to DateRangePicker

* Fix lint error

* Single Date Picker focus fix

* Move calendar date picker to currently focused input element

* Fix issue with onKeyDownTab

* Remove ternary usage

* Fix wrong comparison :)

* Update src/components/SingleDatePicker.jsx

Co-Authored-By: monokrome <github@monokro.me>

* Remove unnecessary `children` prop.

* Remove .cache from .gitignore

* Revert "Remove unnecessary `children` prop."

This reverts commit f5d58d5.

* [SUGGESTION] Push events through onBlur/onTab callbacks

* Call into onClose on focusout for keyboard navigation

* Fix a number of linting issues

* Fix specs.

* Fix issue caused when relatedTarget is sometimes null.

* Add comment explaining why we are using focusout here.

* Fix issues with inconsistencies handling events between date pickers.

* Fix issue where children had no default props.

* Add tests ensuring that DateRangePicker focusout works as expected.

* Add a comment explaining what is happening here.

* Rename to `currentDateOffset`

I'm worried that maybe this feels a bit redundant since anything in
`this.state` should be "current", but it does help to solve the issue w/
naming being weird?...

* Prevent redundancy by renaming local instead of state.

* Only show children if focused. Using `children &&` was odd.

* Using `focused &&` broke a test, but plain `{children}` should work.

* Fix issue using `this.state` as opposed to deconstructed value.

* Version 18.4.1

* Add aria-disabled prop to CalendarDay and CustomizableCalendarDay

* [Dev Deps] update `why-did-you-update`

* Add date offset functionality to DateRangePicker

* Add tests to demonstrate how logic of onDatesChange can affect onFocusChange
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants