chore(form-builder): avoid destroying dom tree when enabling/disablin…

…g change indicator for a field (#2328) Currently a different DOM tree is returned when toggling the change indicator for a field (see https://github.com/sanity-io/sanity/blob/9f38c5a26652a2ad7f1aa69405c724e23a1d60c6/packages/%40sanity/base/src/change-indicators/ChangeIndicator.tsx#L240-L255) As a consequence the react reconciler will destroy the child nodes entirely and recreate when it is toggled. This is especially noticeable when opening array items for editing in which the preview will be destroyed and recreated when opening/closing for edit, often triggering a reload of images etc. This patch makes sure we maintain the same dom structure when toggling change indicators for a field.
sanity-io · Feb 23, 2021 · 4f85843 · 4f85843
1 parent 87053ba
commit 4f85843
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 53 deletions.
diff --git a/packages/@sanity/base/src/change-indicators/ChangeBar.tsx b/packages/@sanity/base/src/change-indicators/ChangeBar.tsx
@@ -40,8 +40,9 @@ export function ChangeBar(props: {
   children: React.ReactNode
   hasFocus: boolean
   isChanged: boolean
+  disabled?: boolean
 }) {
-  const {children, hasFocus, isChanged} = props
+  const {children, hasFocus, isChanged, disabled} = props
 
   const [hover, setHover] = useState(false)
   const {onOpenReviewChanges, isReviewChangesOpen} = React.useContext(ConnectorContext)
@@ -50,48 +51,60 @@ export function ChangeBar(props: {
   const handleMouseLeave = useCallback(() => setHover(false), [])
 
   const tooltip = useMemo(
-    () => (
-      <Tooltip
-        content={
-          <div className={styles.tooltipContent}>
-            <span>Review changes</span>
-          </div>
-        }
-        disabled={!isChanged || isReviewChangesOpen}
-        placement="top"
-      >
-        <div className={styles.wrapper}>
-          <div className={styles.bar} />
+    () =>
+      disabled ? null : (
+        <Tooltip
+          content={
+            <div className={styles.tooltipContent}>
+              <span>Review changes</span>
+            </div>
+          }
+          disabled={!isChanged || isReviewChangesOpen}
+          placement="top"
+        >
+          <div className={styles.wrapper}>
+            <div className={styles.bar} />
 
-          <div className={styles.badge}>
-            <Shape className={styles.badge__shape} />
-            <EditIconSmall className={styles.badge__icon} />
-          </div>
+            <div className={styles.badge}>
+              <Shape className={styles.badge__shape} />
+              <EditIconSmall className={styles.badge__icon} />
+            </div>
 
-          <button
-            tabIndex={isReviewChangesOpen || !isChanged ? -1 : 0}
-            type="button"
-            aria-label="Review changes"
-            onClick={isReviewChangesOpen ? undefined : onOpenReviewChanges}
-            className={styles.hitArea}
-            onMouseEnter={handleMouseEnter}
-            onMouseLeave={handleMouseLeave}
-          />
-        </div>
-      </Tooltip>
-    ),
-    [handleMouseEnter, handleMouseLeave, isReviewChangesOpen, onOpenReviewChanges, isChanged]
+            <button
+              tabIndex={isReviewChangesOpen || !isChanged ? -1 : 0}
+              type="button"
+              aria-label="Review changes"
+              onClick={isReviewChangesOpen ? undefined : onOpenReviewChanges}
+              className={styles.hitArea}
+              onMouseEnter={handleMouseEnter}
+              onMouseLeave={handleMouseLeave}
+            />
+          </div>
+        </Tooltip>
+      ),
+    [
+      handleMouseEnter,
+      handleMouseLeave,
+      isReviewChangesOpen,
+      onOpenReviewChanges,
+      isChanged,
+      disabled,
+    ]
   )
 
   return (
     <div
-      className={classNames(
-        styles.root,
-        hover && styles.hover,
-        hasFocus && styles.focus,
-        isChanged && styles.changed,
-        isReviewChangesOpen && styles.reviewChangesOpen
-      )}
+      className={
+        disabled
+          ? undefined
+          : classNames(
+              styles.root,
+              hover && styles.hover,
+              hasFocus && styles.focus,
+              isChanged && styles.changed,
+              isReviewChangesOpen && styles.reviewChangesOpen
+            )
+      }
     >
       <div className={styles.field}>{children}</div>
       {tooltip}

diff --git a/packages/@sanity/base/src/change-indicators/ChangeIndicator.tsx b/packages/@sanity/base/src/change-indicators/ChangeIndicator.tsx
@@ -34,17 +34,18 @@ const ChangeBarWrapper = memo(function ChangeBarWrapper(
     hasFocus: boolean
     fullPath: Path
     children: React.ReactNode
+    disabled?: boolean
   }
 ) {
-  const {children, className, fullPath, hasFocus, isChanged} = props
+  const {children, className, fullPath, hasFocus, isChanged, disabled} = props
   const layer = useLayer()
   const [hasHover, setHover] = React.useState(false)
   const onMouseEnter = React.useCallback(() => setHover(true), [])
   const onMouseLeave = React.useCallback(() => setHover(false), [])
   const ref = React.useRef<HTMLDivElement | null>(null)
 
   useReporter(
-    `field-${PathUtils.toString(fullPath)}`,
+    disabled ? null : `field-${PathUtils.toString(fullPath)}`,
     () => ({
       element: ref.current!,
       path: props.fullPath,
@@ -59,7 +60,7 @@ const ChangeBarWrapper = memo(function ChangeBarWrapper(
 
   return (
     <div ref={ref} className={className} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave}>
-      <ChangeBar hasFocus={hasFocus} isChanged={isChanged}>
+      <ChangeBar hasFocus={hasFocus} isChanged={isChanged} disabled={disabled}>
         {children}
       </ChangeBar>
     </div>
@@ -122,7 +123,7 @@ export function ChangeIndicatorProvider(props: {
 
 interface CoreProps {
   className?: string
-  hidden?: boolean
+  disabled?: boolean
   fullPath: Path
   compareDeep: boolean
   value: unknown
@@ -133,7 +134,7 @@ interface CoreProps {
 
 export const CoreChangeIndicator = ({
   className,
-  hidden,
+  disabled,
   fullPath,
   value,
   compareValue,
@@ -146,16 +147,13 @@ export const CoreChangeIndicator = ({
     (canCompareShallow(value, compareValue) && value !== compareValue) ||
     (compareDeep && !deepCompare(value, compareValue))
 
-  if (hidden) {
-    return <>{children}</>
-  }
-
   return (
     <ChangeBarWrapper
       className={className}
       isChanged={isChanged}
       fullPath={fullPath}
       hasFocus={hasFocus}
+      disabled={disabled}
     >
       {children}
     </ChangeBarWrapper>
@@ -164,7 +162,7 @@ export const CoreChangeIndicator = ({
 
 export const ChangeIndicatorWithProvidedFullPath = ({
   className,
-  hidden,
+  disabled,
   path,
   value,
   hasFocus,
@@ -180,7 +178,7 @@ export const ChangeIndicatorWithProvidedFullPath = ({
 
   return (
     <CoreChangeIndicator
-      hidden={hidden}
+      disabled={disabled}
       className={className}
       value={value}
       compareValue={PathUtils.get(parentContext.compareValue, path)}
@@ -237,12 +235,9 @@ export const ContextProvidedChangeIndicator = (
   const context = React.useContext(ChangeIndicatorContext)
   const {value, compareValue, path, focusPath, fullPath} = context
 
-  if (disabled) {
-    return <>{children}</>
-  }
-
   return (
     <CoreChangeIndicator
+      disabled={disabled}
       fullPath={fullPath}
       value={value}
       compareValue={compareValue}

diff --git a/packages/@sanity/base/src/change-indicators/noop-tracker.tsx b/packages/@sanity/base/src/change-indicators/noop-tracker.tsx
@@ -12,6 +12,10 @@ export function createNoopTracker<T>() {
   return {
     useReportedValues: noop as () => Reported<T>[],
     Tracker: NoopTracker,
-    useReporter: noop as (id: string, value: T | (() => T), isEqual: IsEqualFunction<T>) => void,
+    useReporter: noop as (
+      id: string | null,
+      value: T | (() => T),
+      isEqual: IsEqualFunction<T>
+    ) => void,
   }
 }