From c72cef305d71863edd47629a282e7fc064b05f84 Mon Sep 17 00:00:00 2001 From: somebody1234 Date: Wed, 20 Nov 2024 21:02:46 +1000 Subject: [PATCH] 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. --- app/common/src/text/english.json | 4 +- .../AriaComponents/Form/components/useForm.ts | 2 +- .../components/AriaComponents/Text/Text.tsx | 17 +- .../AriaComponents/Text/useVisualTooltip.tsx | 8 +- .../src/dashboard/components/Autocomplete.tsx | 20 +- .../dashboard/components/JSONSchemaInput.tsx | 181 ++++++++++-------- .../src/dashboard/data/datalinkSchema.json | 4 +- .../data/datalinks/example-enso-file.datalink | 2 +- 8 files changed, 141 insertions(+), 97 deletions(-) diff --git a/app/common/src/text/english.json b/app/common/src/text/english.json index 5b94930a6c..1fb6aa4eba 100644 --- 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", diff --git a/app/gui/src/dashboard/components/AriaComponents/Form/components/useForm.ts b/app/gui/src/dashboard/components/AriaComponents/Form/components/useForm.ts index a40d1ce5fe..2cd9f56d51 100644 --- 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( errorMap: (issue) => { switch (issue.code) { case 'too_small': - if (issue.minimum === 0) { + if (issue.minimum === 1 && issue.type === 'string') { return { message: getText('arbitraryFieldRequired'), } diff --git a/app/gui/src/dashboard/components/AriaComponents/Text/Text.tsx b/app/gui/src/dashboard/components/AriaComponents/Text/Text.tsx index 3fc708c30a..62cc3573a5 100644 --- 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 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 + readonly triggerRef?: React.RefObject | 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, }) diff --git a/app/gui/src/dashboard/components/Autocomplete.tsx b/app/gui/src/dashboard/components/Autocomplete.tsx index bde95a2b0a..d84f4c81ad 100644 --- 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(props: AutocompleteProps) { const [selectedIndex, setSelectedIndex] = useState(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(null) const inputRef = rawInputRef ?? fallbackInputRef + const containerRef = useRef(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(props: AutocompleteProps) { return (
(props: AutocompleteProps) {
(props: AutocompleteProps) { toggleValue(item) }} > - + {children(item)}
diff --git a/app/gui/src/dashboard/components/JSONSchemaInput.tsx b/app/gui/src/dashboard/components/JSONSchemaInput.tsx index 31e1c66fd6..a3ce89a7ae 100644 --- 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' ? + [{schema.description}] + : [] // 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( -
- 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} - + 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} + +
+ {...errors}
, ) } else { children.push( - - { - const newValue: string = event.currentTarget.value - onChange(newValue) - }} - /> - , +
+ + { + const newValue: string = event.currentTarget.value + onChange(newValue) + }} + /> + + {...errors} +
, ) } break } case 'number': { children.push( - - { - const newValue: number = event.currentTarget.valueAsNumber - if (Number.isFinite(newValue)) { - onChange(newValue) - } - }} - /> - , +
+ + { + const newValue: number = event.currentTarget.valueAsNumber + if (Number.isFinite(newValue)) { + onChange(newValue) + } + }} + /> + + {...errors} +
, ) break } case 'integer': { children.push( - - { - const newValue: number = Math.floor(event.currentTarget.valueAsNumber) - onChange(newValue) - }} - /> - , +
+ + { + const newValue: number = Math.floor(event.currentTarget.valueAsNumber) + onChange(newValue) + }} + /> + + {...errors} +
, ) break } case 'boolean': { children.push( - , +
+ + {...errors} +
, ) 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} /> diff --git a/app/gui/src/dashboard/data/datalinkSchema.json b/app/gui/src/dashboard/data/datalinkSchema.json index 3b9dce0747..cda92b7816 100644 --- 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:///\".", + "description": "Must start with \"enso://Users//\" or \"enso://Teams//\".", "type": "string", - "pattern": "^enso://.+/.*$", + "pattern": "^enso://(?:Users|Teams)/.*/.*$", "format": "enso-file" }, "format": { "title": "Format", "$ref": "#/$defs/Format" } diff --git a/test/Base_Tests/data/datalinks/example-enso-file.datalink b/test/Base_Tests/data/datalinks/example-enso-file.datalink index 6f4edcac64..f210ccaa1a 100644 --- 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" }