fix(form-builder): move handling of ensuring parent item visibility inputs #2329
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). test-studio – ./🔍 Inspect: https://vercel.com/sanity-io/test-studio/C84gpSgJ9cNqgN291b61WpvmFcJ7 perf-studio – ./🔍 Inspect: https://vercel.com/sanity-io/perf-studio/8QrA6TyTUCFrjkkkoCwtuinZKjgk |
| import {useEffect} from 'react' | ||
| import {usePrevious} from './usePrevious' | ||
|
|
||
| export function useDidUpdate<T>(value: T, didUpdate: (current: T, previous: T) => void): void |
There was a problem hiding this comment.
This is a hard thing to name right. Maybe a more explicit naming could be useOnChange?
Also, think it would be good if the signature of the callback would have the same order as shouldComponentUpdate, e.g.
type OnChangeCallback<T> = (prev: T | undefined, next: T) => voidThere was a problem hiding this comment.
I originally considered onChange but landed on useDidUpdate because onChange is usually about events coming from the UI whereas this one is "the value updated since previous render", so it more closely aligns with componentDidUpdate semantics.
Agree on the order of callback params - your suggestion also aligns better with setState, reduce, etc. which expects the previous first.
| import {useEffect, useRef} from 'react' | ||
|
|
||
| export function usePrevious<T>(value: T, initial: T): T | ||
| export function usePrevious<T>(value: T, initial?: T): T | undefined { |
There was a problem hiding this comment.
I don’t understand what this hook is doing. Can you add a description?
There was a problem hiding this comment.
And since it’s only being used by useDidUpdate, maybe this is not needed but should instead be part of useDidUpdate?
There was a problem hiding this comment.
Can you add a description?
Yes, good point. Thanks!
And since it’s only being used by useDidUpdate, maybe this is not needed but should instead be part of useDidUpdate?
It has well defined semantics and is very useful by itself, so it makes sense to have it separate. It might also one day be exported by React and then it can be deleted.
From the FAQ
It’s possible that in the future React will provide a usePrevious Hook out of the box since it’s a relatively common use case
There was a problem hiding this comment.
I still think it’s a little unnecessary to make a new hook for this, since it’s used by another hook and nothing else yet. But it’s not critical of course.
There was a problem hiding this comment.
I generally agree that we shouldn't prematurely make things generic and divide up functions/hooks etc that isn't reused, but since this is such a common hook (I've needed it several times myself) and I don't see the harm in having it separate and readily available for the next person that needs it, I suggest we keep it for now. It's also better separation of concerns and IMO makes the intent behind the resulting code more clear to the reader.
packages/@sanity/form-builder/src/inputs/ArrayInput/item/ArrayInputListItem.css
Outdated
Show resolved
Hide resolved
packages/@sanity/form-builder/src/inputs/ArrayInput/item/ArrayInputGridItem.css
Outdated
Show resolved
Hide resolved
bjoerge
force-pushed
the
fix/ch4500/fix-scroll-jumps
branch
from
79a8e72
to
6057110
Compare
…ty to the inputs that needs it. When a document node value is beeing edited in a dialog we want to make sure the input/preview of the parent/enclosing is scrolled into view in the background. Originally we solved this by responding to changes in the first element of the focus path (e.g. the top most document field name), and whenever this changed, scroll to the dom element with this as the value for `data-focus-path`. As a consequence, opening an item for edit would scroll the editor to the path of the _parent input_ into view, and this would sometimes trigger a scroll jump actually scrolling the field out of view because scrollIntoView would ensure the top edge of the parent input was in view. Instead, what we really wanted was to have the edited _item_ in view. As a bonus, this PR also adds a subtle focus ring on the background element that is currently being edited.
bjoerge
force-pushed
the
fix/ch4500/fix-scroll-jumps
branch
from
6057110
to
3755296
Compare
…ty to the inputs that needs it. (#2329) When a document node value is being edited in a dialog we want to make sure the input/preview of the parent/enclosing is scrolled into view in the background. Originally we solved this by responding to changes in the first element of the focus path (e.g. the top most document field name), and whenever this changed, we would scroll to the dom element with this as the value for `data-focus-path`. As a consequence, opening an item for edit would scroll the editor to the path of the _parent input_ into view, and this would sometimes trigger a scroll jump actually scrolling the field out of view because scrollIntoView would ensure the top edge of the parent input was in view. Instead, what we really want to ensure here is that the edited _item_ is in the view. This fixes the issue by moving the logic for keeping the item in view to the individual input components that needs this behavior. For now, it is only the ArrayOfObjectsInput and the PortableTextInput that requires this. Technically, ImageInput and FileInput also has this behavior, but these don't support deep linking, so it's less of a pressing issue here. As a bonus, this PR also adds a subtle focus ring on the background element that is currently being edited.
When a document node value is beeing edited in a dialog we want to make sure the input/preview of the parent/enclosing is scrolled into view in the background. Originally we solved this by responding to changes in the first element of the focus path (e.g. the top most document field name). Whenever the first element in the focus path changed, we'd then scroll to the dom element that had this as the value for
data-focus-path. As a consequence, opening an item for edit would scroll the editor to the path of the parent input into view, and this would sometimes trigger a scroll jump actually scrolling the field out of view because scrollIntoView would ensure the top edge of the parent input was in view. Instead, what we really wanted was to have the edited item in view.Fixes: #2280
Fixes: #2312
Note for release
As a bonus, this PR also adds a subtle focus ring on the background element that is currently being edited.