Fix Datalink inputs (#11376)

* Stop validating optional Data Link fields * Fix incorrect default value in Datalink input * Stop `Autocomplete` fields from opening automatically without `autoFocus` * Increase E2E test timeout * Fix E2E test race condition? * Fix error message for empty string input * Highlight active Datalink input * Show description when Datalink input is invalid * Fix Datalink input unfocusing when errors appear * Fix `enso://` path error text * Fix hover display of autocomplete items * Move `Autocomplete` tooltip above Dropdown container * Update Enso path validation * Update test file and Enso File datalink regex --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Bypassing failing tests.
2024-12-22 08:21:41 +03:00 · 2024-11-20 21:02:46 +10:00 · 2024-11-20 21:02:46 +10:00 · c72cef305d
commit c72cef305d
parent 7327df88c3
8 changed files with 141 additions and 97 deletions
--- a/app/common/src/text/english.json
+++ b/app/common/src/text/english.json
@ -804,8 +804,8 @@
  "arbitraryFieldInvalid": "This field is invalid",
  "arbitraryFieldTooShort": "This field is too short",
  "arbitraryFieldTooLong": "This field is too long",
-  "arbitraryFieldTooSmall": "The value is too small, the minimum is $0",
-  "arbitraryFieldTooLarge": "The value is too large, the maximum is $0",
+  "arbitraryFieldTooSmall": "The value must be greater than $0",
+  "arbitraryFieldTooLarge": "The value must be less than $0",
  "arbitraryFieldNotEqual": "This field is not equal to another field",
  "arbitraryFieldNotMatch": "This field does not match the pattern",
  "arbitraryFieldNotMatchAny": "This field does not match any of the patterns",
--- a/app/gui/src/dashboard/components/AriaComponents/Form/components/useForm.ts
+++ b/app/gui/src/dashboard/components/AriaComponents/Form/components/useForm.ts
@ -85,7 +85,7 @@ export function useForm<Schema extends types.TSchema, SubmitResult = void>(
          errorMap: (issue) => {
            switch (issue.code) {
              case 'too_small':
-                if (issue.minimum === 0) {
+                if (issue.minimum === 1 && issue.type === 'string') {
                  return {
                    message: getText('arbitraryFieldRequired'),
                  }
--- a/app/gui/src/dashboard/components/AriaComponents/Text/Text.tsx
+++ b/app/gui/src/dashboard/components/AriaComponents/Text/Text.tsx
@ -17,8 +17,11 @@ export interface TextProps
  readonly elementType?: keyof HTMLElementTagNameMap
  readonly lineClamp?: number
  readonly tooltip?: React.ReactElement | string | false | null
+  readonly tooltipTriggerRef?: React.RefObject<HTMLElement>
  readonly tooltipDisplay?: visualTooltip.VisualTooltipProps['display']
  readonly tooltipPlacement?: aria.Placement
+  readonly tooltipOffset?: number
+  readonly tooltipCrossOffset?: number
 }

 export const TEXT_STYLE = twv.tv({
@ -134,8 +137,11 @@ export const Text = forwardRef(function Text(props: TextProps, ref: React.Ref<HT
    balance,
    elementType: ElementType = 'span',
    tooltip: tooltipElement = children,
+    tooltipTriggerRef,
    tooltipDisplay = 'whenOverflowing',
    tooltipPlacement,
+    tooltipOffset,
+    tooltipCrossOffset,
    textSelection,
    disableLineHeightCompensation = false,
    ...ariaProps
@ -176,9 +182,18 @@ export const Text = forwardRef(function Text(props: TextProps, ref: React.Ref<HT
  const { tooltip, targetProps } = visualTooltip.useVisualTooltip({
    isDisabled: isTooltipDisabled(),
    targetRef: textElementRef,
+    triggerRef: tooltipTriggerRef,
    display: tooltipDisplay,
    children: tooltipElement,
-    ...(tooltipPlacement ? { overlayPositionProps: { placement: tooltipPlacement } } : {}),
+    ...(tooltipPlacement || tooltipOffset != null ?
+      {
+        overlayPositionProps: {
+          ...(tooltipPlacement && { placement: tooltipPlacement }),
+          ...(tooltipOffset != null && { offset: tooltipOffset }),
+          ...(tooltipCrossOffset != null && { crossOffset: tooltipCrossOffset }),
+        },
+      }
+    : {}),
  })

  return (
--- a/app/gui/src/dashboard/components/AriaComponents/Text/useVisualTooltip.tsx
+++ b/app/gui/src/dashboard/components/AriaComponents/Text/useVisualTooltip.tsx
@ -18,10 +18,11 @@ export interface VisualTooltipProps
  readonly children: React.ReactNode
  readonly className?: string
  readonly targetRef: React.RefObject<HTMLElement>
+  readonly triggerRef?: React.RefObject<HTMLElement> | undefined
  readonly isDisabled?: boolean
  readonly overlayPositionProps?: Pick<
    aria.AriaPositionProps,
-    'containerPadding' | 'offset' | 'placement'
+    'containerPadding' | 'crossOffset' | 'offset' | 'placement'
  >
  /**
   * Determines when the tooltip should be displayed.
@ -56,6 +57,7 @@ export function useVisualTooltip(props: VisualTooltipProps): VisualTooltipReturn
  const {
    children,
    targetRef,
+    triggerRef = targetRef,
    className,
    isDisabled = false,
    overlayPositionProps = {},
@ -70,6 +72,7 @@ export function useVisualTooltip(props: VisualTooltipProps): VisualTooltipReturn
  const {
    containerPadding = 0,
    offset = DEFAULT_OFFSET,
+    crossOffset = 0,
    placement = 'bottom',
  } = overlayPositionProps

@ -115,8 +118,9 @@ export function useVisualTooltip(props: VisualTooltipProps): VisualTooltipReturn
  const { overlayProps, updatePosition } = aria.useOverlayPosition({
    isOpen: state.isOpen,
    overlayRef: popoverRef,
-    targetRef,
+    targetRef: triggerRef,
    offset,
+    crossOffset,
    placement,
    containerPadding,
  })
--- a/app/gui/src/dashboard/components/Autocomplete.tsx
+++ b/app/gui/src/dashboard/components/Autocomplete.tsx
@ -1,6 +1,5 @@
 /** @file A select menu with a dropdown. */
 import {
-  useEffect,
  useMemo,
  useRef,
  useState,
@ -92,22 +91,15 @@ export default function Autocomplete<T>(props: AutocompleteProps<T>) {
  const [selectedIndex, setSelectedIndex] = useState<number | null>(null)
  const valuesSet = useMemo(() => new Set(values), [values])
  const canEditText = setText != null && values.length === 0
-  // We are only interested in the initial value of `canEditText` in effects.
-  const canEditTextRef = useRef(canEditText)
  const isMultipleAndCustomValue = multiple === true && text != null
  const matchingItems = useMemo(
    () => (text == null ? items : items.filter((item) => matches(item, text))),
    [items, matches, text],
  )

-  useEffect(() => {
-    if (!canEditTextRef.current) {
-      setIsDropdownVisible(true)
-    }
-  }, [])
-
  const fallbackInputRef = useRef<HTMLFieldSetElement>(null)
  const inputRef = rawInputRef ?? fallbackInputRef
+  const containerRef = useRef<HTMLDivElement>(null)

  // This type is a little too wide but it is unavoidable.
  /** Set values, while also changing the input text. */
@ -184,6 +176,7 @@ export default function Autocomplete<T>(props: AutocompleteProps<T>) {
  return (
    <div className={twJoin('relative isolate h-6 w-full', isDropdownVisible && 'z-1')}>
      <div
+        ref={containerRef}
        onKeyDown={onKeyDown}
        className={twMerge(
          'absolute w-full grow transition-colors',
@ -259,7 +252,7 @@ export default function Autocomplete<T>(props: AutocompleteProps<T>) {
              <div
                key={itemToKey(item)}
                className={twMerge(
-                  'text relative cursor-pointer whitespace-nowrap px-input-x last:rounded-b-xl hover:bg-hover-bg',
+                  'text relative min-w-max cursor-pointer whitespace-nowrap rounded-full px-input-x last:rounded-b-xl hover:bg-hover-bg',
                  valuesSet.has(item) && 'bg-hover-bg',
                  index === selectedIndex && 'bg-black/5',
                )}
@ -271,7 +264,12 @@ export default function Autocomplete<T>(props: AutocompleteProps<T>) {
                  toggleValue(item)
                }}
              >
-                <Text truncate="1" className="w-full" tooltipPlacement="left">
+                <Text
+                  truncate="1"
+                  className="w-full"
+                  tooltipPlacement="top"
+                  tooltipTriggerRef={containerRef}
+                >
                  {children(item)}
                </Text>
              </div>
--- a/app/gui/src/dashboard/components/JSONSchemaInput.tsx
+++ b/app/gui/src/dashboard/components/JSONSchemaInput.tsx
@ -51,8 +51,13 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
    schema.format === 'enso-secret'
  const { data: secrets } = useBackendQuery(remoteBackend, 'listSecrets', [], { enabled: isSecret })
  const autocompleteItems = isSecret ? secrets?.map((secret) => secret.path) ?? null : null
-  const validityClassName =
-    isAbsent || getValidator(path)(value) ? 'border-primary/20' : 'border-red-700/60'
+  const isInvalid = !isAbsent && !getValidator(path)(value)
+  const validationErrorClassName =
+    isInvalid && 'border border-danger focus:border-danger focus:outline-danger'
+  const errors =
+    isInvalid && 'description' in schema && typeof schema.description === 'string' ?
+      [<Text className="px-2 text-danger">{schema.description}</Text>]
+    : []

  // NOTE: `enum` schemas omitted for now as they are not yet used.
  if ('const' in schema) {
@ -66,100 +71,120 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
          if ('format' in schema && schema.format === 'enso-secret') {
            const isValid = typeof value === 'string' && value !== ''
            children.push(
-              <div className={twMerge('w-full rounded-default border-0.5', validityClassName)}>
-                <Autocomplete
-                  items={autocompleteItems ?? []}
-                  itemToKey={(item) => item}
-                  placeholder={getText('enterSecretPath')}
-                  matches={(item, text) => item.toLowerCase().includes(text.toLowerCase())}
-                  values={isValid ? [value] : []}
-                  setValues={(values) => {
-                    onChange(values[0] ?? '')
-                  }}
-                  text={autocompleteText}
-                  setText={setAutocompleteText}
+              <div className="flex flex-col">
+                <div
+                  className={twMerge(
+                    'w-full rounded-default border-0.5 border-primary/20 outline-offset-2 transition-[border-color,outline] duration-200 focus:border-primary/50 focus:outline focus:outline-2 focus:outline-offset-0 focus:outline-primary',
+                    validationErrorClassName,
+                  )}
                >
-                  {(item) => item}
-                </Autocomplete>
+                  <Autocomplete
+                    items={autocompleteItems ?? []}
+                    itemToKey={(item) => item}
+                    placeholder={getText('enterSecretPath')}
+                    matches={(item, text) => item.toLowerCase().includes(text.toLowerCase())}
+                    values={isValid ? [value] : []}
+                    setValues={(values) => {
+                      onChange(values[0] ?? '')
+                    }}
+                    text={autocompleteText}
+                    setText={setAutocompleteText}
+                  >
+                    {(item) => item}
+                  </Autocomplete>
+                </div>
+                {...errors}
              </div>,
            )
          } else {
            children.push(
-              <FocusRing>
-                <Input
-                  type="text"
-                  readOnly={readOnly}
-                  value={typeof value === 'string' ? value : ''}
-                  size={1}
-                  className={twMerge(
-                    'focus-child h-6 w-full grow rounded-input border-0.5 bg-transparent px-2 read-only:read-only',
-                    validityClassName,
-                  )}
-                  placeholder={getText('enterText')}
-                  onChange={(event) => {
-                    const newValue: string = event.currentTarget.value
-                    onChange(newValue)
-                  }}
-                />
-              </FocusRing>,
+              <div className="flex flex-col">
+                <FocusRing>
+                  <Input
+                    type="text"
+                    readOnly={readOnly}
+                    value={typeof value === 'string' ? value : ''}
+                    size={1}
+                    className={twMerge(
+                      'focus-child h-6 w-full grow rounded-input border-0.5 border-primary/20 bg-transparent px-2 outline-offset-2 transition-[border-color,outline] duration-200 read-only:read-only focus:border-primary/50 focus:outline focus:outline-2 focus:outline-offset-0 focus:outline-primary',
+                      validationErrorClassName,
+                    )}
+                    placeholder={getText('enterText')}
+                    onChange={(event) => {
+                      const newValue: string = event.currentTarget.value
+                      onChange(newValue)
+                    }}
+                  />
+                </FocusRing>
+                {...errors}
+              </div>,
            )
          }
          break
        }
        case 'number': {
          children.push(
-            <FocusRing>
-              <Input
-                type="number"
-                readOnly={readOnly}
-                value={typeof value === 'number' ? value : ''}
-                size={1}
-                className={twMerge(
-                  'focus-child h-6 w-full grow rounded-input border-0.5 bg-transparent px-2 read-only:read-only',
-                  validityClassName,
-                )}
-                placeholder={getText('enterNumber')}
-                onChange={(event) => {
-                  const newValue: number = event.currentTarget.valueAsNumber
-                  if (Number.isFinite(newValue)) {
-                    onChange(newValue)
-                  }
-                }}
-              />
-            </FocusRing>,
+            <div className="flex flex-col">
+              <FocusRing>
+                <Input
+                  type="number"
+                  readOnly={readOnly}
+                  value={typeof value === 'number' ? value : ''}
+                  size={1}
+                  className={twMerge(
+                    'focus-child h-6 w-full grow rounded-input border-0.5 border-primary/20 bg-transparent px-2 outline-offset-2 transition-[border-color,outline] duration-200 read-only:read-only focus:border-primary/50 focus:outline focus:outline-2 focus:outline-offset-0 focus:outline-primary',
+                    validationErrorClassName,
+                  )}
+                  placeholder={getText('enterNumber')}
+                  onChange={(event) => {
+                    const newValue: number = event.currentTarget.valueAsNumber
+                    if (Number.isFinite(newValue)) {
+                      onChange(newValue)
+                    }
+                  }}
+                />
+              </FocusRing>
+              {...errors}
+            </div>,
          )
          break
        }
        case 'integer': {
          children.push(
-            <FocusRing>
-              <Input
-                type="number"
-                readOnly={readOnly}
-                value={typeof value === 'number' ? value : ''}
-                size={1}
-                className={twMerge(
-                  'focus-child h-6 w-full grow rounded-input border-0.5 bg-transparent px-2 read-only:read-only',
-                  validityClassName,
-                )}
-                placeholder={getText('enterInteger')}
-                onChange={(event) => {
-                  const newValue: number = Math.floor(event.currentTarget.valueAsNumber)
-                  onChange(newValue)
-                }}
-              />
-            </FocusRing>,
+            <div className="flex flex-col">
+              <FocusRing>
+                <Input
+                  type="number"
+                  readOnly={readOnly}
+                  value={typeof value === 'number' ? value : ''}
+                  size={1}
+                  className={twMerge(
+                    'focus-child h-6 w-full grow rounded-input border-0.5 border-primary/20 bg-transparent px-2 outline-offset-2 transition-[border-color,outline] duration-200 read-only:read-only focus:border-primary/50 focus:outline focus:outline-2 focus:outline-offset-0 focus:outline-primary',
+                    validationErrorClassName,
+                  )}
+                  placeholder={getText('enterInteger')}
+                  onChange={(event) => {
+                    const newValue: number = Math.floor(event.currentTarget.valueAsNumber)
+                    onChange(newValue)
+                  }}
+                />
+              </FocusRing>
+              {...errors}
+            </div>,
          )
          break
        }
        case 'boolean': {
          children.push(
-            <Checkbox
-              name="input"
-              isReadOnly={readOnly}
-              isSelected={typeof value === 'boolean' && value}
-              onChange={onChange}
-            />,
+            <div className="flex flex-col">
+              <Checkbox
+                name="input"
+                isReadOnly={readOnly}
+                isSelected={typeof value === 'boolean' && value}
+                onChange={onChange}
+              />
+              {...errors}
+            </div>,
          )
          break
        }
@ -186,7 +211,7 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
              >
                {propertyDefinitions.map((definition) => {
                  const { key, schema: childSchema } = definition
-                  const isOptional = !requiredProperties.includes(key)
+                  const isOptional = !requiredProperties.includes(key) || isAbsent
                  const isPresent = !isAbsent && value != null && key in value
                  return constantValueOfSchema(defs, childSchema).length === 1 ?
                      null
@ -250,7 +275,7 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
                                newValue = unsafeValue!
                              }
                              const fullObject =
-                                value ?? constantValueOfSchema(defs, childSchema, true)[0]
+                                value ?? constantValueOfSchema(defs, schema, true)[0]
                              onChange(
                                (
                                  typeof fullObject === 'object' &&
@ -346,6 +371,7 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
              path={selectedChildPath}
              getValidator={getValidator}
              noBorder={noChildBorder}
+              isAbsent={isAbsent}
              value={value}
              onChange={onChange}
            />
@ -364,6 +390,7 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) {
          path={`${path}/allOf/${i}`}
          getValidator={getValidator}
          noBorder={noChildBorder}
+          isAbsent={isAbsent}
          value={value}
          onChange={onChange}
        />
--- a/app/gui/src/dashboard/data/datalinkSchema.json
+++ b/app/gui/src/dashboard/data/datalinkSchema.json
@ -117,9 +117,9 @@
        "libraryName": { "const": "Standard.Base" },
        "path": {
          "title": "Path",
-          "description": "Must start with \"enso://<organization-name>/\".",
+          "description": "Must start with \"enso://Users/<username>/\" or \"enso://Teams/<team name>/\".",
          "type": "string",
-          "pattern": "^enso://.+/.*$",
+          "pattern": "^enso://(?:Users|Teams)/.*/.*$",
          "format": "enso-file"
        },
        "format": { "title": "Format", "$ref": "#/$defs/Format" }
--- a/test/Base_Tests/data/datalinks/example-enso-file.datalink
+++ b/test/Base_Tests/data/datalinks/example-enso-file.datalink
@ -1,5 +1,5 @@
 {
    "type": "Enso_File",
    "libraryName": "Standard.Base",
-    "path": "enso://PLACEHOLDER_ORG_NAME/PLACEHOLDER_PATH"
+    "path": "enso://Teams/PLACEHOLDER_TEAM_NAME/PLACEHOLDER_PATH"
 }