From d44b7ac6c2d651b01949c21dda3a31c2a014e954 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 29 Apr 2025 15:06:42 +0200 Subject: [PATCH] 1-3651/single value inputs (#9859) Adds an "add value" with popover input for single-value fields (numerical and semver operators). The implementation re-uses the popover from the multi-value constraint operators, so I've extracted it for re-use. All current input types: image For the new one, opening the popover when there's a value will pre-select the value, so you can override it by typing immediately: image Buttons look pretty identical: image ## Discussion points ### Input type I haven't set an input type anywhere on the popover yet. In theory, we could use input type "number" for numerical inputs and I think it's worth looking at that, but we don't do in the old implementation either. I've added a task for it. ### Weird esc handling This implementation uses a chip for the button/value display for the single. In almost all cases it works exactly as I'd expect, but closing the popover with esc moves your focus to the top of `body`. Unfortunately, this isn't something we can address directly (trust me, I've tried), but the good news is that this was fixed in mui v6. The current major is v7, so we probably want to update before too long, which will also fix this. More info in the MUI docs: https://mui.com/material-ui/migration/upgrade-to-v6/#chip I think that for the single value entry, losing focus on esc is a fair tradeoff because it handles swapping states etc so gracefully. For the multi-value operators, however, esc is the only way to close the popover, so losing focus when you do that is not acceptable to me. As such, I'll leave the multi-value input as a button for now instead. (It's also totally fine because the button never updates or needs to change). --- .../AddSingleValueWidget.tsx | 65 +++++++++ .../AddValuesPopover.tsx | 127 ++++++++++++++++++ .../AddValuesWidget.tsx | 105 ++------------- .../EditableConstraint.tsx | 24 +++- .../FeatureStrategyConstraints/ValueList.tsx | 8 +- 5 files changed, 225 insertions(+), 104 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesPopover.tsx diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx new file mode 100644 index 0000000000..5cc82a35fe --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx @@ -0,0 +1,65 @@ +import Add from '@mui/icons-material/Add'; +import { styled } from '@mui/material'; +import { forwardRef, useImperativeHandle, useRef, useState } from 'react'; +import { ValueChip } from './ValueList'; +import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; + +const StyledChip = styled(ValueChip, { + shouldForwardProp: (prop) => prop !== 'hasValue', +})<{ hasValue: boolean }>(({ theme, hasValue }) => ({ + color: hasValue ? 'inherit' : theme.palette.primary.main, + '.MuiChip-icon': { + transform: 'translateX(50%)', + fill: theme.palette.primary.main, + height: theme.fontSizes.smallerBody, + width: theme.fontSizes.smallerBody, + }, +})); + +interface AddValuesProps { + onAddValue: (newValue: string) => void; + removeValue: () => void; + currentValue?: string; +} + +export const AddSingleValueWidget = forwardRef( + ({ currentValue, onAddValue, removeValue }, ref) => { + const [open, setOpen] = useState(false); + const positioningRef = useRef(null); + useImperativeHandle( + ref, + () => positioningRef.current as HTMLDivElement, + ); + + const handleAdd = (newValue: string, { setError }: OnAddActions) => { + if (newValue.length > 100) { + setError('Values cannot be longer than 100 characters'); + return; + } + + onAddValue(newValue); + setError(''); + setOpen(false); + }; + + return ( + <> + setOpen(true)} + icon={currentValue ? undefined : } + onDelete={currentValue ? removeValue : undefined} + /> + setOpen(false)} + /> + + ); + }, +); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesPopover.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesPopover.tsx new file mode 100644 index 0000000000..c51c997b54 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesPopover.tsx @@ -0,0 +1,127 @@ +import { Button, Popover, styled, TextField } from '@mui/material'; +import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; +import { type FC, useId, useRef, useState } from 'react'; + +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), +})); + +export type OnAddActions = { + setError: (error: string) => void; + clearInput: () => void; +}; + +type AddValuesProps = { + onAdd: (newValue: string, actions: OnAddActions) => void; + initialValue?: string; + open: boolean; + anchorEl: HTMLElement | null; + onClose: () => void; +}; + +export const AddValuesPopover: FC = ({ + initialValue, + onAdd, + anchorEl, + open, + onClose, +}) => { + const [inputValue, setInputValue] = useState(initialValue || ''); + const [error, setError] = useState(''); + const inputRef = useRef(null); + const inputId = useId(); + + return ( + { + if (inputValue && !initialValue?.trim()) { + // if the input value is not empty and the current value is + // empty or whitespace + setInputValue(''); + } else if (inputValue) { + // select the text in the input field + inputRef?.current?.select(); + } + }} + disableScrollLock + anchorEl={anchorEl} + onClose={onClose} + anchorOrigin={{ + vertical: 'bottom', + horizontal: 'left', + }} + transformOrigin={{ + vertical: 'top', + horizontal: 'left', + }} + > +
{ + e.stopPropagation(); + e.preventDefault(); + if (!inputValue?.trim()) { + setError('Value cannot be empty or whitespace'); + return; + } else { + onAdd(inputValue, { + setError, + clearInput: () => setInputValue(''), + }); + } + }} + > + {error && {error}} + + + + + { + setInputValue(e.target.value); + setError(''); + }} + size='small' + variant='standard' + fullWidth + inputRef={inputRef} + autoFocus + /> + + +
+ + ); +}; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx index 5ea4cc1682..1d43370803 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx @@ -1,16 +1,11 @@ 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 { styled } from '@mui/material'; +import { forwardRef, useImperativeHandle, useRef, useState } from 'react'; import { parseParameterStrings } from 'utils/parseParameter'; import { baseChipStyles } from './ValueList'; +import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; +// todo: MUI v6 / v7 upgrade: consider changing this to a Chip to align with the rest of the values and the single value selector. There was a fix introduced in v6 that makes you not lose focus on pressing esc: https://mui.com/material-ui/migration/upgrade-to-v6/#chip talk to Thomas for more info. const AddValuesButton = styled('button')(({ theme }) => ({ ...baseChipStyles(theme), color: theme.palette.primary.main, @@ -31,32 +26,6 @@ const AddValuesButton = styled('button')(({ theme }) => ({ cursor: 'pointer', })); -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; } @@ -64,17 +33,16 @@ interface AddValuesProps { 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 handleAdd = ( + inputValues: string, + { setError, clearInput }: OnAddActions, + ) => { const newValues = parseParameterStrings(inputValues); if (newValues.length === 0) { @@ -88,9 +56,8 @@ export const AddValuesWidget = forwardRef( } onAddValues(newValues); - setInputValues(''); + clearInput(); setError(''); - inputRef?.current?.focus(); }; return ( @@ -103,59 +70,13 @@ export const AddValuesWidget = forwardRef( 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 5602b60a41..83d0783cec 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -26,8 +26,10 @@ import { ReactComponent as CaseInsensitiveIcon } from 'assets/icons/case-insensi import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; import { AddValuesWidget } from './AddValuesWidget'; import { ResolveInput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput'; + import { ReactComponent as EqualsIcon } from 'assets/icons/constraint-equals.svg'; import { ReactComponent as NotEqualsIcon } from 'assets/icons/constraint-not-equals.svg'; +import { AddSingleValueWidget } from './AddSingleValueWidget'; const Container = styled('article')(({ theme }) => ({ '--padding': theme.spacing(2), @@ -151,16 +153,16 @@ const StyledCaseSensitiveIcon = styled(CaseSensitiveIcon)(({ theme }) => ({ fill: 'currentcolor', })); -const CaseButton = styled(StyledButton)(({ theme }) => ({ - display: 'grid', - placeItems: 'center', -})); - const OPERATORS_WITH_ADD_VALUES_WIDGET = [ 'IN_OPERATORS_FREETEXT', 'STRING_OPERATORS_FREETEXT', ]; +const SINGLE_VALUE_OPERATORS = [ + 'NUM_OPERATORS_SINGLE_VALUE', + 'SEMVER_OPERATORS_SINGLE_VALUE', +]; + type Props = { constraint: IConstraint; localConstraint: IConstraint; @@ -212,9 +214,10 @@ export const EditableConstraint: FC = ({ useState(false); const deleteButtonRef = useRef(null); const addValuesButtonRef = useRef(null); + const showSingleValueButton = SINGLE_VALUE_OPERATORS.includes(input); const showAddValuesButton = OPERATORS_WITH_ADD_VALUES_WIDGET.includes(input); - const showInputField = !showAddValuesButton; + const showInputField = input.includes('LEGAL_VALUES'); /* 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 @@ -359,6 +362,15 @@ export const EditableConstraint: FC = ({ /> ) : null} + {showSingleValueButton ? ( + { + setValue(newValue); + }} + removeValue={() => setValue('')} + currentValue={localConstraint.value} + /> + ) : null} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx index af696590c3..27cae4602a 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx @@ -27,9 +27,9 @@ export const baseChipStyles = (theme: Theme) => ({ transition: 'all 0.3s ease', }); -const ValueChipBase = styled( +export const ValueChip = styled( forwardRef((props, ref) => ( - + } /> )), )(({ theme }) => ({ ...baseChipStyles(theme), @@ -44,9 +44,6 @@ const ValueChipBase = styled( '& .MuiChip-deleteIcon': { marginRight: theme.spacing(1), }, -})); - -const ValueChip = styled(ValueChipBase)(({ theme }) => ({ svg: { fill: theme.palette.secondary.dark, borderRadius: '50%', @@ -105,7 +102,6 @@ export const ValueList: FC> = ({ whiteSpace: 'normal', }, }} - deleteIcon={} label={value} onDelete={() => { nextFocusTarget(index)?.focus();