fix: release dateTime picker bugs #8403
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
No changes to documentation |
|
Component Testing Report Updated Jan 30, 2025 12:43 PM (UTC) ❌ Failed Tests (1) -- expand for details
|
⚡️ Editor Performance ReportUpdated Thu, 30 Jan 2025 12:44:35 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
69fc97c to
6ed6816
Compare
76a8c95 to
f817f57
Compare
…ing dateTime for scheduled release to be set when created
|
I haven't reviewed the code itself, but how does this play with timezones? Was this something you double checked? (specifically related to putting a date for earlier than the current time) |
| useImperativeHandle<HTMLInputElement | null, HTMLInputElement | null>( | ||
| forwardedRef, | ||
| () => ref.current, | ||
| ) | ||
|
|
||
| useEffect(() => setReferenceElement(ref.current), []) |
There was a problem hiding this comment.
So this was something new I learned doing this:
Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.
From here
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 the referenceElement happen after the initial render cycle, so that the ref.current value is accurate and aligned with the react state model
There was a problem hiding this comment.
- Add why the useEffect is there as a code comment
There was a problem hiding this comment.
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.
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.
| portal | ||
| content={ | ||
| <Box overflow="auto"> | ||
| <FocusLock onDeactivation={handleDeactivation}> | ||
| {inputValue && isPastDisabled && isPast(new Date(inputValue)) && ( |
There was a problem hiding this comment.
could we move this outside to not have it in the render? use a useMemo / etc?
There was a problem hiding this comment.
- Memoize in
DateTimeInput
| status: 'warning', | ||
| title: tRelease('schedule-dialog.publish-date-in-past-warning'), | ||
| }) | ||
| setRerenderDialog((cur) => cur + 1) |
There was a problem hiding this comment.
ooh, do we need this? This feels like something of a hack 😬
I don't understand why we need it, but I don't quite like it 🥲
|
|
||
| const handleConfirmSchedule = useCallback(async () => { | ||
| if (!publishAt) return | ||
|
|
||
| if (isScheduledDateInPast()) { | ||
| // rerender dialog to recalculate isScheduledDateInPast | ||
| setRerenderDialog((cur) => cur + 1) |
There was a problem hiding this comment.
Same as this comment 🙏
I don't know if this is the best way of going about it? What is the use case, sorry I don't understand why we are doing it and it might be obvious 😅
There was a problem hiding this comment.
So this is for the edge case that you select a time that is in the future, say 1 min in the future, but you take 1 minute to complete the dialog before submitting. The handleSubmit callbacks check again against the current time, if they see that you are now in the past, there needs to be a way of getting the component to rerender so that the submit button now shows as disabled and has a warning or tooltip. Since no actual existing state has changed, I added this state, to flush a rerender of the component and cause isScheduledDateInPast to be rerun and repainted. Does this make sense - happy to sync
There was a problem hiding this comment.
Speaking with Jordan, we should add a comment about the scenarios and edge cases that rely on this and why and if we find a better solution in the future but otherwise let's keep this as is :)
packages/sanity/src/core/releases/tool/components/releaseCTAButtons/ReleaseScheduleButton.tsx
Show resolved
Hide resolved
| cancelButton?: Omit<ComponentProps<typeof Button>, 'fontSize' | 'padding'> | ||
| confirmButton?: Omit<ComponentProps<typeof Button>, 'fontSize' | 'padding'> |
There was a problem hiding this comment.
Concerned about this change because it will impact other things and other parts of the studio. Should we be making this change now? On Corel?
There was a problem hiding this comment.
Yeh this was also my concern. Perhaps to avoid fallout from this with COREL I can just render a warning elsewhere in the dialog and we can come back to this once opt in-out is completed
There was a problem hiding this comment.
I think that might be wiser, or at least pluck this from here and put it directly into a next improvement (which we would update on our dialog once that came back around)
Good callout - I've got a followup PR that resolves the timezone issues that builds on this one |
Description
Fixes a number of issues:
ReleaseForm,CopyToNewReleaseandCreateReleaseDialogWhat to review
Dialogto use the ui component's Buttons (so we can use tooltips on these buttons). Think there could be any issues here?Testing
Adjusted existing tests; will fast-follow new tests for date in the past scenarios
Notes for release
N/A