-
Notifications
You must be signed in to change notification settings - Fork 448
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: release dateTime picker bugs #8403
Closed
Closed
Changes from 40 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
1b1f979
refactor: removing unneeded state for printed datetime
jordanl17 33c0d92
refactor: supporting disabling of just input for DateTimeInput; allow…
jordanl17 f124491
fix: setting seconds to zero for all schedule times
jordanl17 3c4ed80
fix: disabling picking dates in the past for scheduled releases
jordanl17 79af045
fix: schedule confirm does not allow historical dateTimes for schedul…
jordanl17 4101540
fix: updating type picker date prevents historical dates
jordanl17 6b40f3d
fix: default the release time picker to check if the current date is …
jordanl17 ea138a4
fix: creating new release prevents historical dates for publish; inpu…
jordanl17 dd69dfb
refactor: minor refactor of the create release date components
jordanl17 9c5fa42
test: fixing existing release date tests
jordanl17 3979bbc
refactor: further refactor of ReleaseTypePicker
jordanl17 1dace9d
test: increasing timeout on ReleaseDialog tests
jordanl17 5201cc8
refactor: removing unneeded state for printed datetime
jordanl17 e44a508
refactor: supporting disabling of just input for DateTimeInput; allow…
jordanl17 02d5856
fix: setting seconds to zero for all schedule times
jordanl17 43c3831
fix: disabling picking dates in the past for scheduled releases
jordanl17 3fc0bff
fix: schedule confirm does not allow historical dateTimes for schedul…
jordanl17 5554fe9
fix: updating type picker date prevents historical dates
jordanl17 9455514
fix: default the release time picker to check if the current date is …
jordanl17 86d8980
fix: creating new release prevents historical dates for publish; inpu…
jordanl17 ed9cf71
refactor: minor refactor of the create release date components
jordanl17 e25d61f
test: fixing existing release date tests
jordanl17 ba22da0
refactor: further refactor of ReleaseTypePicker
jordanl17 b733ba9
test: increasing timeout on ReleaseDialog tests
jordanl17 e63f574
Merge branch 'fix/corel-disabled-time-picker' of github.com:sanity-io…
jordanl17 3dcfebb
refactor: removing unneeded state for printed datetime
jordanl17 af04a40
refactor: supporting disabling of just input for DateTimeInput; allow…
jordanl17 10ebbb0
fix: setting seconds to zero for all schedule times
jordanl17 0c5aec1
fix: disabling picking dates in the past for scheduled releases
jordanl17 96f08e5
fix: schedule confirm does not allow historical dateTimes for schedul…
jordanl17 0868bff
fix: updating type picker date prevents historical dates
jordanl17 2357fb9
fix: default the release time picker to check if the current date is …
jordanl17 3856dbb
fix: creating new release prevents historical dates for publish; inpu…
jordanl17 b3a4253
refactor: minor refactor of the create release date components
jordanl17 c832fe4
test: fixing existing release date tests
jordanl17 d7ecbf9
refactor: further refactor of ReleaseTypePicker
jordanl17 a9a93e7
test: increasing timeout on ReleaseDialog tests
jordanl17 6ba2259
Merge branch 'fix/corel-disabled-time-picker' of github.com:sanity-io…
jordanl17 d3868c7
fix: changing to scheduled uses intended date first
jordanl17 808f868
refactor: better handling of temporal dates going into past; i18n of …
jordanl17 255250b
refactor: reverting back to sanity UI Buttons in ui component Dialog
jordanl17 47cb673
refactor: memoization optimization in DateTimeInput
jordanl17 ef495d0
refactor: improving code commentary around rerenderDialog state to ha…
jordanl17 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import {CalendarIcon} from '@sanity/icons' | ||
import {Box, Flex, LayerProvider, useClickOutsideEvent} from '@sanity/ui' | ||
import {Box, Card, Flex, LayerProvider, Text, useClickOutsideEvent} from '@sanity/ui' | ||
import {isPast} from 'date-fns' | ||
import { | ||
type FocusEvent, | ||
type ForwardedRef, | ||
forwardRef, | ||
type KeyboardEvent, | ||
useCallback, | ||
useEffect, | ||
useImperativeHandle, | ||
useRef, | ||
useState, | ||
|
@@ -14,6 +16,7 @@ import FocusLock from 'react-focus-lock' | |
|
||
import {Button} from '../../../../ui-components/button/Button' | ||
import {Popover} from '../../../../ui-components/popover/Popover' | ||
import {useTranslation} from '../../../i18n' | ||
import {type CalendarProps} from './calendar/Calendar' | ||
import {type CalendarLabels} from './calendar/types' | ||
import {DatePicker} from './DatePicker' | ||
|
@@ -35,6 +38,7 @@ export interface DateTimeInputProps { | |
monthPickerVariant?: CalendarProps['monthPickerVariant'] | ||
padding?: number | ||
disableInput?: boolean | ||
isPastDisabled?: boolean | ||
} | ||
|
||
export const DateTimeInput = forwardRef(function DateTimeInput( | ||
|
@@ -53,17 +57,24 @@ export const DateTimeInput = forwardRef(function DateTimeInput( | |
constrainSize = true, | ||
monthPickerVariant, | ||
padding, | ||
disableInput, | ||
isPastDisabled, | ||
...rest | ||
} = props | ||
const {t} = useTranslation() | ||
const popoverRef = useRef<HTMLDivElement | null>(null) | ||
const ref = useRef<HTMLInputElement | null>(null) | ||
const buttonRef = useRef(null) | ||
|
||
const [referenceElement, setReferenceElement] = useState<HTMLInputElement | null>(null) | ||
|
||
useImperativeHandle<HTMLInputElement | null, HTMLInputElement | null>( | ||
forwardedRef, | ||
() => ref.current, | ||
) | ||
|
||
useEffect(() => setReferenceElement(ref.current), []) | ||
|
||
const [isPickerOpen, setPickerOpen] = useState(false) | ||
|
||
useClickOutsideEvent( | ||
|
@@ -104,7 +115,7 @@ export const DateTimeInput = forwardRef(function DateTimeInput( | |
<LazyTextInput | ||
ref={ref} | ||
{...rest} | ||
readOnly={readOnly} | ||
readOnly={disableInput || readOnly} | ||
value={inputValue} | ||
onChange={onInputChange} | ||
suffix={ | ||
|
@@ -116,17 +127,24 @@ export const DateTimeInput = forwardRef(function DateTimeInput( | |
<Popover | ||
constrainSize={constrainSize} | ||
data-testid="date-input-dialog" | ||
referenceElement={referenceElement} | ||
portal | ||
content={ | ||
<Box overflow="auto"> | ||
<FocusLock onDeactivation={handleDeactivation}> | ||
{inputValue && isPastDisabled && isPast(new Date(inputValue)) && ( | ||
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. could we move this outside to not have it in the render? use a useMemo / etc? 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.
|
||
<Card margin={1} padding={2} radius={2} shadow={1} tone="critical"> | ||
<Text size={1}>{t('inputs.dateTime.past-date-warning')}</Text> | ||
</Card> | ||
)} | ||
<DatePicker | ||
monthPickerVariant={monthPickerVariant} | ||
calendarLabels={calendarLabels} | ||
selectTime={selectTime} | ||
timeStep={timeStep} | ||
onKeyUp={handleKeyUp} | ||
value={value} | ||
isPastDisabled={isPastDisabled} | ||
onChange={onChange} | ||
padding={padding} | ||
/> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
72 changes: 72 additions & 0 deletions
72
packages/sanity/src/core/releases/components/ScheduleDatePicker.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import {EarthGlobeIcon} from '@sanity/icons' | ||
import {Flex} from '@sanity/ui' | ||
import {format, isValid, parse} from 'date-fns' | ||
import {useCallback, useMemo} from 'react' | ||
|
||
import {Button} from '../../../ui-components/button' | ||
import {MONTH_PICKER_VARIANT} from '../../components/inputs/DateInputs/calendar/Calendar' | ||
import {type CalendarLabels} from '../../components/inputs/DateInputs/calendar/types' | ||
import {DateTimeInput} from '../../components/inputs/DateInputs/DateTimeInput' | ||
import {getCalendarLabels} from '../../form/inputs/DateInputs' | ||
import {useTranslation} from '../../i18n/hooks/useTranslation' | ||
import useDialogTimeZone from '../../scheduledPublishing/hooks/useDialogTimeZone' | ||
import useTimeZone from '../../scheduledPublishing/hooks/useTimeZone' | ||
|
||
interface ScheduleDatePickerProps { | ||
initialValue: Date | ||
onChange: (date: Date) => void | ||
} | ||
|
||
const inputDateFormat = 'PP HH:mm' | ||
|
||
export const ScheduleDatePicker = ({ | ||
initialValue: inputValue, | ||
onChange, | ||
}: ScheduleDatePickerProps) => { | ||
const {t} = useTranslation() | ||
const {timeZone} = useTimeZone() | ||
const {dialogTimeZoneShow} = useDialogTimeZone() | ||
|
||
const handlePublishAtCalendarChange = (date: Date | null) => { | ||
if (!date) return | ||
|
||
onChange(date) | ||
} | ||
|
||
const handlePublishAtInputChange = useCallback( | ||
(event: React.FocusEvent<HTMLInputElement>) => { | ||
const date = event.currentTarget.value | ||
const parsedDate = parse(date, inputDateFormat, new Date()) | ||
|
||
if (isValid(parsedDate)) onChange(parsedDate) | ||
}, | ||
[onChange], | ||
) | ||
|
||
const calendarLabels: CalendarLabels = useMemo(() => getCalendarLabels(t), [t]) | ||
|
||
return ( | ||
<Flex flex={1} justify="space-between"> | ||
<DateTimeInput | ||
selectTime | ||
monthPickerVariant={MONTH_PICKER_VARIANT.carousel} | ||
onChange={handlePublishAtCalendarChange} | ||
onInputChange={handlePublishAtInputChange} | ||
calendarLabels={calendarLabels} | ||
value={inputValue} | ||
inputValue={format(inputValue, inputDateFormat)} | ||
constrainSize={false} | ||
padding={0} | ||
isPastDisabled | ||
/> | ||
|
||
<Button | ||
icon={EarthGlobeIcon} | ||
mode="bleed" | ||
size="default" | ||
text={`${timeZone.abbreviation}`} | ||
onClick={dialogTimeZoneShow} | ||
/> | ||
</Flex> | ||
) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why do we need this? 🤔
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.
So this was something new I learned doing this:
This is I think symptomatic that the sanity UI Popover should accept a react ref as
referenceElement
rather than a HTML element.But by putting this into state and setting in the
useEffect
, it's ensured that the updates to thereferenceElement
happen after the initial render cycle, so that theref.current
value is accurate and aligned with the react state modelThere 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.
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.
I see, might need to add a task to improve this at some, it just feels bad that we need to add a useeffect where the only thing we are doing is this. Which might be fine but since useEffects tend to tank performance I'm aways just 👁️ when I see them
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.
Speaking with Jordan, we should create a story for updating the popover to work as intended instead of having us rely on workarounds from the studio side. This fine for now we adding a code comment there.