-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: correctly format date/time in RTL #7423
Changes from all commits
bbe6191
0716ad5
76b8d0e
47e2af5
ef0b637
b3e2f70
7b77f1e
fd5a902
d9b69a8
a9f732e
8df5ad6
679e355
bedf713
c2df442
9080188
2b5fb13
91173c3
79a8da3
b923904
ae03f3b
2bbde19
81560bd
dad9ceb
ec8fe1d
a863e18
891dce9
4b56394
1a9eaa9
f50b4b1
e39f1ea
05ed94d
ccf7b85
b1915c2
af3ab18
50bb4df
1bd410e
623219d
ccf7def
ccce891
2e682e2
6efe67f
0fdb98d
ab2a67a
83d2727
9e74c07
e48425b
8e10996
13d47a9
4d1adff
9acdf22
c02ec95
e564b9c
77a6bb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,48 @@ | ||
import {createFocusManager, getFocusableTreeWalker} from '@react-aria/focus'; | ||
import {DateFieldState, DatePickerState, DateRangePickerState} from '@react-stately/datepicker'; | ||
import {FocusableElement, KeyboardEvent, RefObject} from '@react-types/shared'; | ||
import {mergeProps} from '@react-aria/utils'; | ||
import {mergeProps, useLayoutEffect} from '@react-aria/utils'; | ||
import {useLocale} from '@react-aria/i18n'; | ||
import {useMemo} from 'react'; | ||
import {useMemo, useRef} from 'react'; | ||
import {usePress} from '@react-aria/interactions'; | ||
|
||
export function useDatePickerGroup(state: DatePickerState | DateRangePickerState | DateFieldState, ref: RefObject<Element | null>, disableArrowNavigation?: boolean) { | ||
let {direction} = useLocale(); | ||
let focusManager = useMemo(() => createFocusManager(ref), [ref]); | ||
let segments = useRef<FocusableElement[]>(undefined); | ||
useLayoutEffect(() => { | ||
if (ref?.current) { | ||
|
||
let update = () => { | ||
if (ref.current) { | ||
// TODO: For now, just querying this list of elements. However, it's possible that either through hooks or RAC that some users may include other focusable items that they would want to able to keyboard navigate to. In that case, we might want to utilize focusableElements in isFocusable.ts | ||
let editableSegments: NodeListOf<Element> | undefined = ref.current?.querySelectorAll('span[role="spinbutton"], span[role="textbox"], button, div[role="spinbutton"], div[role="textbox"]'); | ||
|
||
let segmentsArr = Array.from(editableSegments as NodeListOf<Element>).filter(Boolean).map(node => { | ||
return { | ||
element: node as FocusableElement, | ||
rectX: node.getBoundingClientRect().left | ||
}; | ||
}); | ||
|
||
let orderedSegments = segmentsArr.sort((a, b) => a.rectX - b.rectX).map((item => item.element)); | ||
segments.current = orderedSegments; | ||
} | ||
}; | ||
|
||
update(); | ||
|
||
let observer = new MutationObserver(update); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the MutationObserver? When would the segments change order without the date picker itself re-rendering? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm good point,,,i think it should be fine if we were to get rid of it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was for if users added more buttons to control the field, Yihui had an example with a I don't know if those buttons would ever be removed or added dynamically There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right, I think it was from the example in this issue #7645 |
||
observer.observe(ref.current, { | ||
subtree: true, | ||
childList: true | ||
}); | ||
|
||
return () => { | ||
observer.disconnect(); | ||
}; | ||
} | ||
}, []); | ||
|
||
// Open the popover on alt + arrow down | ||
let onKeyDown = (e: KeyboardEvent) => { | ||
|
@@ -31,7 +65,21 @@ export function useDatePickerGroup(state: DatePickerState | DateRangePickerState | |
e.preventDefault(); | ||
e.stopPropagation(); | ||
if (direction === 'rtl') { | ||
focusManager.focusNext(); | ||
if (segments.current) { | ||
let orderedSegments = segments.current; | ||
let target = e.target as FocusableElement; | ||
let index = orderedSegments.indexOf(target); | ||
|
||
if (index === 0) { | ||
target = orderedSegments[0] || target; | ||
} else { | ||
target = orderedSegments[index - 1] || target; | ||
} | ||
|
||
if (target) { | ||
target.focus(); | ||
} | ||
} | ||
} else { | ||
focusManager.focusPrevious(); | ||
} | ||
|
@@ -40,7 +88,24 @@ export function useDatePickerGroup(state: DatePickerState | DateRangePickerState | |
e.preventDefault(); | ||
e.stopPropagation(); | ||
if (direction === 'rtl') { | ||
focusManager.focusPrevious(); | ||
if (segments.current) { | ||
let orderedSegments = segments.current; | ||
let target = e.target as FocusableElement; | ||
let index = orderedSegments.indexOf(target); | ||
|
||
if (index === orderedSegments.length - 1) { | ||
target = orderedSegments[orderedSegments.length - 1] || target; | ||
} else { | ||
target = orderedSegments[index - 1] || target; | ||
} | ||
|
||
|
||
target = orderedSegments[index + 1] || target; | ||
|
||
if (target) { | ||
target.focus(); | ||
} | ||
} | ||
} else { | ||
focusManager.focusNext(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added for DateRangePicker because things were looking a little funky without it. wraps around each datefield. see codepen for reproduction
i could update this so that it only applies in rtl locales but it also doesn't seem to have any affect for ltr locales.