From 59a6ef46e8fe205df22cac0648ab3ddf7d155448 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Wed, 20 Dec 2023 15:36:23 +0100 Subject: [PATCH] fix: separate concerns for ConstraintAccordionList (#5701) ## Problem The ConstraintAccordionList component was used in multiple places: * Playground * Segment form * StrategyExecution * Change requests * Create strategy * Edit strategy This is problematic because some of the views are just pure visual representations, and other views allow you to interact with and edit the constraints. This causes a situation where the visual representation needs to be aware of the implementation details of editing and mutating constraints. In addition the ConstraintAccordionList is not just a pure rendering of the list, it also keeps internal state on when to show the create button and optional headers. This is makes it hard to make changes when stylings need to be subtly different across components. ## Solution Taking on the full refactor for this is out of scope, but it's unfortunate that the ConstraintAccordionList needs all this internal state. For now I split out the list into it's own component called ConstraintList. I gathered the functions needed for editing and mutating the constraints in a reusable hook and isolated the version of the list used in the new feature strategy edit / create components into it's own component so that the changes in layout will not affect anything else. Ideally we should try to move towards a future where the components don't keep internal state like this but clear boundaries and purposes for the use. --- .../AutocompleteBox/AutocompleteBox.tsx | 4 +- .../ConstraintAccordionList.tsx | 295 ++++++++---------- ...FeatureStrategyConstraintAccordionList.tsx | 190 +++++++++++ .../FeatureStrategyConstraints.tsx | 4 +- .../NewFeatureStrategyCreate.test.tsx | 2 +- frontend/src/hooks/useWeakMap.ts | 2 +- 6 files changed, 321 insertions(+), 176 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx diff --git a/frontend/src/component/common/AutocompleteBox/AutocompleteBox.tsx b/frontend/src/component/common/AutocompleteBox/AutocompleteBox.tsx index b42f43269b..913993a3b8 100644 --- a/frontend/src/component/common/AutocompleteBox/AutocompleteBox.tsx +++ b/frontend/src/component/common/AutocompleteBox/AutocompleteBox.tsx @@ -107,9 +107,9 @@ export const AutocompleteBox = ({ }, }, }} - placeholder={placeHolder} + placeholder={label} onFocus={() => setPlaceholder('')} - onBlur={() => setPlaceholder('Add Segments')} + onBlur={() => setPlaceholder(label)} /> ); }; diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx index e6332bab54..7c4845a01a 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx @@ -1,19 +1,23 @@ -import React, { forwardRef, Fragment, useImperativeHandle } from 'react'; -import { Box, Button, styled, Tooltip, Typography } from '@mui/material'; -import { Add, HelpOutline } from '@mui/icons-material'; +import React, { + forwardRef, + Fragment, + Ref, + RefObject, + useImperativeHandle, +} from 'react'; +import { Box, Button, styled, Tooltip } from '@mui/material'; +import { HelpOutline } from '@mui/icons-material'; import { IConstraint } from 'interfaces/strategy'; import { ConstraintAccordion } from 'component/common/ConstraintAccordion/ConstraintAccordion'; import produce from 'immer'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; -import { useWeakMap } from 'hooks/useWeakMap'; +import { IUseWeakMap, useWeakMap } from 'hooks/useWeakMap'; import { objectId } from 'utils/objectId'; import { createEmptyConstraint } from 'component/common/ConstraintAccordion/ConstraintAccordionList/createEmptyConstraint'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { StrategySeparator } from 'component/common/StrategySeparator/StrategySeparator'; -import { useUiFlag } from 'hooks/useUiFlag'; -import { HelpIcon } from 'component/common/HelpIcon/HelpIcon'; -interface IConstraintAccordionListProps { +export interface IConstraintAccordionListProps { constraints: IConstraint[]; setConstraints?: React.Dispatch>; showCreateButton?: boolean; @@ -66,12 +70,35 @@ const StyledAddCustomLabel = styled('div')(({ theme }) => ({ display: 'flex', })); -const StyledHelpIconBox = styled(Box)(({ theme }) => ({ - display: 'flex', - alignItems: 'center', - marginTop: theme.spacing(1), - marginBottom: theme.spacing(1), -})); +export const useConstraintAccordionList = ( + setConstraints: + | React.Dispatch> + | undefined, + ref: React.RefObject, +) => { + const state = useWeakMap(); + const { context } = useUnleashContext(); + + const addConstraint = + setConstraints && + ((contextName: string) => { + const constraint = createEmptyConstraint(contextName); + state.set(constraint, { editing: true, new: true }); + setConstraints((prev) => [...prev, constraint]); + }); + + useImperativeHandle(ref, () => ({ + addConstraint, + })); + + const onAdd = + addConstraint && + (() => { + addConstraint(context[0].name); + }); + + return { onAdd, state, context }; +}; export const ConstraintAccordionList = forwardRef< IConstraintAccordionListRef | undefined, @@ -81,151 +108,15 @@ export const ConstraintAccordionList = forwardRef< { constraints, setConstraints, showCreateButton, showLabel = true }, ref, ) => { - const state = useWeakMap< - IConstraint, - IConstraintAccordionListItemState - >(); - const { context } = useUnleashContext(); - - const newStrategyConfiguration = useUiFlag('newStrategyConfiguration'); - - const addConstraint = - setConstraints && - ((contextName: string) => { - const constraint = createEmptyConstraint(contextName); - state.set(constraint, { editing: true, new: true }); - setConstraints((prev) => [...prev, constraint]); - }); - - useImperativeHandle(ref, () => ({ - addConstraint, - })); - - const onAdd = - addConstraint && - (() => { - addConstraint(context[0].name); - }); - - const onEdit = - setConstraints && - ((constraint: IConstraint) => { - state.set(constraint, { editing: true }); - }); - - const onRemove = - setConstraints && - ((index: number) => { - const constraint = constraints[index]; - state.set(constraint, {}); - setConstraints( - produce((draft) => { - draft.splice(index, 1); - }), - ); - }); - - const onSave = - setConstraints && - ((index: number, constraint: IConstraint) => { - state.set(constraint, {}); - setConstraints( - produce((draft) => { - draft[index] = constraint; - }), - ); - }); - - const onCancel = (index: number) => { - const constraint = constraints[index]; - state.get(constraint)?.new && onRemove?.(index); - state.set(constraint, {}); - }; + const { onAdd, state, context } = useConstraintAccordionList( + setConstraints, + ref as RefObject, + ); if (context.length === 0) { return null; } - if (newStrategyConfiguration) { - return ( - - - - Constraints - - - Constraints are advanced - targeting rules that you can - use to enable a feature - toggle for a subset of your - users. Read more about - constraints{' '} - - here - - - - } - /> - - {constraints.map((constraint, index) => ( - - 0} - show={ - - } - /> - - - ))} - - - } - /> - - ); - } - return ( } /> - {constraints.map((constraint, index) => ( - - 0} - show={} - /> - - - ))} + >; + state: IUseWeakMap; +} + +export const ConstraintList = forwardRef< + IConstraintAccordionListRef | undefined, + IConstraintList +>(({ constraints, setConstraints, state }, ref) => { + const { context } = useUnleashContext(); + + const onEdit = + setConstraints && + ((constraint: IConstraint) => { + state.set(constraint, { editing: true }); + }); + + const onRemove = + setConstraints && + ((index: number) => { + const constraint = constraints[index]; + state.set(constraint, {}); + setConstraints( + produce((draft) => { + draft.splice(index, 1); + }), + ); + }); + + const onSave = + setConstraints && + ((index: number, constraint: IConstraint) => { + state.set(constraint, {}); + setConstraints( + produce((draft) => { + draft[index] = constraint; + }), + ); + }); + + const onCancel = (index: number) => { + const constraint = constraints[index]; + state.get(constraint)?.new && onRemove?.(index); + state.set(constraint, {}); + }; + + if (context.length === 0) { + return null; + } + + return ( + + {constraints.map((constraint, index) => ( + + {console.log(state.get(constraint))} + 0} + show={} + /> + + + ))} + + ); +}); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx new file mode 100644 index 0000000000..710b75de6e --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx @@ -0,0 +1,190 @@ +import React, { forwardRef, RefObject } from 'react'; +import { Box, Button, styled, Tooltip, Typography } from '@mui/material'; +import { Add, HelpOutline } from '@mui/icons-material'; +import { IConstraint } from 'interfaces/strategy'; + +import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; + +import { useUiFlag } from 'hooks/useUiFlag'; +import { HelpIcon } from 'component/common/HelpIcon/HelpIcon'; +import { + ConstraintList, + IConstraintAccordionListRef, + useConstraintAccordionList, +} from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList'; + +interface IConstraintAccordionListProps { + constraints: IConstraint[]; + setConstraints?: React.Dispatch>; + showCreateButton?: boolean; + /* Add "constraints" title on the top - default `true` */ + showLabel?: boolean; +} +export const constraintAccordionListId = 'constraintAccordionListId'; + +const StyledContainer = styled('div')({ + width: '100%', + display: 'flex', + flexDirection: 'column', +}); + +const StyledHelpWrapper = styled(Tooltip)(({ theme }) => ({ + marginLeft: theme.spacing(0.75), + height: theme.spacing(1.5), +})); + +const StyledHelp = styled(HelpOutline)(({ theme }) => ({ + fill: theme.palette.action.active, + [theme.breakpoints.down(860)]: { + display: 'none', + }, +})); + +const StyledConstraintLabel = styled('p')(({ theme }) => ({ + marginBottom: theme.spacing(1), + color: theme.palette.text.secondary, +})); + +const StyledAddCustomLabel = styled('div')(({ theme }) => ({ + marginTop: theme.spacing(1), + marginBottom: theme.spacing(1), + color: theme.palette.text.primary, + display: 'flex', +})); + +const StyledHelpIconBox = styled(Box)(({ theme }) => ({ + display: 'flex', + alignItems: 'center', + marginTop: theme.spacing(1), + marginBottom: theme.spacing(1), +})); + +export const FeatureStrategyConstraintAccordionList = forwardRef< + IConstraintAccordionListRef | undefined, + IConstraintAccordionListProps +>( + ( + { constraints, setConstraints, showCreateButton, showLabel = true }, + ref, + ) => { + const { onAdd, state, context } = useConstraintAccordionList( + setConstraints, + ref as RefObject, + ); + const newStrategyConfiguration = useUiFlag('newStrategyConfiguration'); + + if (context.length === 0) { + return null; + } + + if (newStrategyConfiguration) { + return ( + + + + Constraints + + + Constraints are advanced + targeting rules that you can + use to enable a feature + toggle for a subset of your + users. Read more about + constraints{' '} + + here + + + + } + /> + + + + + } + /> + + ); + } + + return ( + + 0 && showLabel + } + show={ + + Constraints + + } + /> + + + +

Add any number of constraints

+ + + + + + + + + } + /> + + ); + }, +); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx index a68b30f602..75edf60ad7 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx @@ -1,11 +1,11 @@ import { IConstraint, IFeatureStrategy } from 'interfaces/strategy'; import React, { useMemo } from 'react'; -import { ConstraintAccordionList } from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList'; import { UPDATE_FEATURE_STRATEGY, CREATE_FEATURE_STRATEGY, } from 'component/providers/AccessProvider/permissions'; import { useHasProjectEnvironmentAccess } from 'hooks/useHasAccess'; +import { FeatureStrategyConstraintAccordionList } from './FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList'; interface IFeatureStrategyConstraintsProps { projectId: string; @@ -46,7 +46,7 @@ export const FeatureStrategyConstraints = ({ ); return ( - { const doneEl = screen.getByText('Done'); fireEvent.click(doneEl); - const selectElement = screen.getByPlaceholderText('Add Segments'); + const selectElement = screen.getByPlaceholderText('Select segments'); fireEvent.mouseDown(selectElement); const optionElement = await screen.findByText(expectedSegmentName); diff --git a/frontend/src/hooks/useWeakMap.ts b/frontend/src/hooks/useWeakMap.ts index 64c7a792a8..fb9448e7b2 100644 --- a/frontend/src/hooks/useWeakMap.ts +++ b/frontend/src/hooks/useWeakMap.ts @@ -1,6 +1,6 @@ import { useState, useCallback, useRef, useMemo } from 'react'; -interface IUseWeakMap { +export interface IUseWeakMap { set: (k: K, v: V) => void; get: (k: K) => V | undefined; }