From 08d0907d89dfb510040f51dd8dfdbf1f39ba5155 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 23 Apr 2025 10:59:10 +0200 Subject: [PATCH] Always show the value list + hide "add values" on non-free text entries (#9817) Removes the condition to hide the value list if we use legal values. In doing so, I also realized that focus handling when you delete the last item in the constraint values list doesn't work if the add values button isn't there (which it shouldn't be for legal values and more). So I've hidden the add values button when it doesn't do anythnig helpful (or for cases where we don't have designs yet). In cases where you don't have the add values button and you delete the last constraint value, we'll move the focus to the "delete constraint" button (that was easier than making sure we pass refs all the way down into the operator select, but we can change that later). To facilitate this (refs coming from the parent component), I refactored the value list component to accept the add values widget as a child (and extracted it to its own file). --- .../AddValuesWidget.tsx | 166 +++++++++++++++ .../EditableConstraint.tsx | 51 ++++- .../FeatureStrategyConstraints/ValueList.tsx | 197 +----------------- 3 files changed, 217 insertions(+), 197 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx new file mode 100644 index 0000000000..8c8a4535c5 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx @@ -0,0 +1,166 @@ +import Add from '@mui/icons-material/Add'; +import { Button, Popover, styled, TextField } from '@mui/material'; +import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; +import { + forwardRef, + useId, + useImperativeHandle, + useRef, + useState, +} from 'react'; +import { parseParameterStrings } from 'utils/parseParameter'; + +const AddValuesButton = styled('button')(({ theme }) => ({ + color: theme.palette.primary.main, + svg: { + fill: theme.palette.primary.main, + height: theme.fontSizes.smallerBody, + width: theme.fontSizes.smallerBody, + }, + border: 'none', + borderRadius: theme.shape.borderRadiusExtraLarge, + display: 'flex', + flexFlow: 'row nowrap', + whiteSpace: 'nowrap', + gap: theme.spacing(0.25), + alignItems: 'center', + padding: theme.spacing(0.5, 1.5, 0.5, 1.5), + height: 'auto', + transition: 'all 0.3s ease', + outline: `1px solid #0000`, + background: theme.palette.background.elevation1, + ':hover, :focus-visible': { + background: theme.palette.background.elevation1, + outlineColor: theme.palette.secondary.dark, + }, +})); + +const StyledPopover = styled(Popover)(({ theme }) => ({ + '& .MuiPaper-root': { + borderRadius: theme.shape.borderRadiusLarge, + border: `1px solid ${theme.palette.divider}`, + padding: theme.spacing(2), + width: '250px', + }, +})); + +const StyledTextField = styled(TextField)(({ theme }) => ({ + flexGrow: 1, +})); + +const InputRow = styled('div')(({ theme }) => ({ + display: 'flex', + gap: theme.spacing(1), + alignItems: 'start', + width: '100%', +})); + +const ErrorMessage = styled('div')(({ theme }) => ({ + color: theme.palette.error.main, + fontSize: theme.typography.caption.fontSize, + marginBottom: theme.spacing(1), +})); + +interface AddValuesProps { + onAddValues: (newValues: string[]) => void; +} + +export const AddValuesWidget = forwardRef( + ({ onAddValues }, ref) => { + const [open, setOpen] = useState(false); + const [inputValues, setInputValues] = useState(''); + const [error, setError] = useState(''); + const positioningRef = useRef(null); + useImperativeHandle( + ref, + () => positioningRef.current as HTMLButtonElement, + ); + const inputRef = useRef(null); + const inputId = useId(); + + const handleAdd = () => { + const newValues = parseParameterStrings(inputValues); + + if (newValues.length === 0) { + setError('Values cannot be empty'); + return; + } + + if (newValues.some((v) => v.length > 100)) { + setError('Values cannot be longer than 100 characters'); + return; + } + + onAddValues(newValues); + setInputValues(''); + setError(''); + inputRef?.current?.focus(); + }; + + return ( + <> + setOpen(true)} + type='button' + > + + Add values + + setOpen(false)} + anchorOrigin={{ + vertical: 'bottom', + horizontal: 'left', + }} + transformOrigin={{ + vertical: 'top', + horizontal: 'left', + }} + > +
{ + e.stopPropagation(); + e.preventDefault(); + handleAdd(); + }} + > + {error && {error}} + + + + + { + setInputValues(e.target.value); + setError(''); + }} + size='small' + variant='standard' + fullWidth + inputRef={inputRef} + autoFocus + /> + + +
+ + + ); + }, +); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx index 389b9268a9..6e6c1daf12 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -30,7 +30,7 @@ import type { IUnleashContextDefinition, } from 'interfaces/context'; import type { IConstraint } from 'interfaces/strategy'; -import { useEffect, useState, type FC } from 'react'; +import { useEffect, useRef, useState, type FC } from 'react'; import { oneOf } from 'utils/oneOf'; import { CURRENT_TIME_CONTEXT_FIELD, @@ -43,6 +43,7 @@ import { ValueList } from './ValueList'; import { ReactComponent as CaseSensitiveIcon } from 'assets/icons/case-sensitive.svg'; import { ReactComponent as CaseInsensitiveIcon } from 'assets/icons/case-insensitive.svg'; import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; +import { AddValuesWidget } from './AddValuesWidget'; const Container = styled('article')(({ theme }) => ({ '--padding': theme.spacing(2), @@ -143,6 +144,11 @@ const CaseButton = styled(StyledButton)(({ theme }) => ({ placeItems: 'center', })); +const OPERATORS_WITH_ADD_VALUES_WIDGET = [ + 'IN_OPERATORS_FREETEXT', + 'STRING_OPERATORS_FREETEXT', +]; + type Props = { localConstraint: IConstraint; setContextName: (contextName: string) => void; @@ -190,6 +196,10 @@ export const EditableConstraint: FC = ({ const { contextName, operator } = localConstraint; const [showCaseSensitiveButton, setShowCaseSensitiveButton] = useState(false); + const deleteButtonRef = useRef(null); + const addValuesButtonRef = useRef(null); + const showAddValuesButton = + OPERATORS_WITH_ADD_VALUES_WIDGET.includes(input); /* We need a special case to handle the currentTime context field. Since this field will be the only one to allow DATE_BEFORE and DATE_AFTER operators @@ -410,17 +420,40 @@ export const EditableConstraint: FC = ({ ) : null} - {!input.includes('LEGAL_VALUES') && ( - - )} + + addValuesButtonRef.current ?? + deleteButtonRef.current + } + > + {showAddValuesButton ? ( + { + // todo (`addEditStrategy`): move deduplication logic higher up in the context handling + const combinedValues = new Set([ + ...(localConstraint.values || []), + ...newValues, + ]); + setValuesWithRecord( + Array.from(combinedValues), + ); + }} + /> + ) : null} + - + diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx index a63b10fc60..883c00bc9e 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx @@ -1,23 +1,6 @@ -import Add from '@mui/icons-material/Add'; import Clear from '@mui/icons-material/Clear'; -import { - Button, - Chip, - type ChipProps, - Popover, - styled, - TextField, -} from '@mui/material'; -import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; -import { - type FC, - forwardRef, - useId, - useImperativeHandle, - useRef, - useState, -} from 'react'; -import { parseParameterStrings } from 'utils/parseParameter'; +import { Chip, type ChipProps, styled } from '@mui/material'; +import { type FC, forwardRef, type PropsWithChildren, useRef } from 'react'; const ValueListWrapper = styled('div')(({ theme }) => ({ display: 'flex', @@ -72,181 +55,28 @@ const ValueChip = styled(ValueChipBase)(({ theme }) => ({ }, })); -const AddValuesButton = styled('button')(({ theme }) => ({ - color: theme.palette.primary.main, - svg: { - fill: theme.palette.primary.main, - height: theme.fontSizes.smallerBody, - width: theme.fontSizes.smallerBody, - }, - border: 'none', - borderRadius: theme.shape.borderRadiusExtraLarge, - display: 'flex', - flexFlow: 'row nowrap', - whiteSpace: 'nowrap', - gap: theme.spacing(0.25), - alignItems: 'center', - padding: theme.spacing(0.5, 1.5, 0.5, 1.5), - height: 'auto', - transition: 'all 0.3s ease', - outline: `1px solid #0000`, - background: theme.palette.background.elevation1, - ':hover, :focus-visible': { - background: theme.palette.background.elevation1, - outlineColor: theme.palette.secondary.dark, - }, -})); - -const StyledPopover = styled(Popover)(({ theme }) => ({ - '& .MuiPaper-root': { - borderRadius: theme.shape.borderRadiusLarge, - border: `1px solid ${theme.palette.divider}`, - padding: theme.spacing(2), - width: '250px', - }, -})); - -const StyledTextField = styled(TextField)(({ theme }) => ({ - flexGrow: 1, -})); - -const InputRow = styled('div')(({ theme }) => ({ - display: 'flex', - gap: theme.spacing(1), - alignItems: 'start', - width: '100%', -})); - -const ErrorMessage = styled('div')(({ theme }) => ({ - color: theme.palette.error.main, - fontSize: theme.typography.caption.fontSize, - marginBottom: theme.spacing(1), -})); - -interface AddValuesProps { - onAddValues: (values: string[]) => void; -} - -const AddValues = forwardRef( - ({ onAddValues }, ref) => { - const [open, setOpen] = useState(false); - const [inputValues, setInputValues] = useState(''); - const [error, setError] = useState(''); - const positioningRef = useRef(null); - useImperativeHandle( - ref, - () => positioningRef.current as HTMLButtonElement, - ); - const inputRef = useRef(null); - const inputId = useId(); - - const handleAdd = () => { - const newValues = parseParameterStrings(inputValues); - - if (newValues.length === 0) { - setError('Values cannot be empty'); - return; - } - - if (newValues.some((v) => v.length > 100)) { - setError('Values cannot be longer than 100 characters'); - return; - } - - onAddValues(newValues); - setInputValues(''); - setError(''); - inputRef?.current?.focus(); - }; - - return ( - <> - setOpen(true)} - type='button' - > - - Add values - - setOpen(false)} - anchorOrigin={{ - vertical: 'bottom', - horizontal: 'left', - }} - transformOrigin={{ - vertical: 'top', - horizontal: 'left', - }} - > -
{ - e.stopPropagation(); - e.preventDefault(); - handleAdd(); - }} - > - {error && {error}} - - - - - { - setInputValues(e.target.value); - setError(''); - }} - size='small' - variant='standard' - fullWidth - inputRef={inputRef} - autoFocus - /> - - -
- - - ); - }, -); - type Props = { values: string[] | undefined; removeValue: (index: number) => void; setValues: (values: string[]) => void; + // the element that should receive focus when all value chips are deleted + getExternalFocusTarget: () => HTMLElement | null; }; -export const ValueList: FC = ({ +export const ValueList: FC> = ({ values = [], removeValue, - setValues, + getExternalFocusTarget, + children, }) => { const constraintElementRefs: React.MutableRefObject< (HTMLDivElement | null)[] > = useRef([]); - const addValuesButtonRef = useRef(null); const nextFocusTarget = (deletedIndex: number) => { if (deletedIndex === values.length - 1) { if (deletedIndex === 0) { - return addValuesButtonRef.current; + return getExternalFocusTarget(); } else { return constraintElementRefs.current[deletedIndex - 1]; } @@ -255,11 +85,6 @@ export const ValueList: FC = ({ } }; - const handleAddValues = (newValues: string[]) => { - const combinedValues = uniqueValues([...(values || []), ...newValues]); - setValues(combinedValues); - }; - return ( @@ -279,11 +104,7 @@ export const ValueList: FC = ({ ))} - + {children} ); }; - -const uniqueValues = (values: T[]): T[] => { - return Array.from(new Set(values)); -};