Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-25 00:07:47 +01:00
Code Issues Projects Releases Wiki Activity

fix: separate concerns for ConstraintAccordionList (#5701)

Browse Source 
## 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.
This commit is contained in:
Fredrik Strand Oseberg 2023-12-20 15:36:23 +01:00 committed by GitHub
parent 6d0e32810c
commit 59a6ef46e8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 321 additions and 176 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
frontend/src/component/common/AutocompleteBox/AutocompleteBox.tsx
View File

@ -107,9 +107,9 @@ export const AutocompleteBox = ({
},
},
}}
placeholder={placeHolder}
placeholder={label}
onFocus={() => setPlaceholder('')}
onBlur={() => setPlaceholder('Add Segments')}
onBlur={() => setPlaceholder(label)}
/>
);
};

295
frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx
View File

@ -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<React.SetStateAction<IConstraint[]>>;
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<React.SetStateAction<IConstraint[]>>
| undefined,
ref: React.RefObject<IConstraintAccordionListRef>,
) => {
const state = useWeakMap<IConstraint, IConstraintAccordionListItemState>();
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<IConstraintAccordionListRef>,
);
if (context.length === 0) {
return null;
}
if (newStrategyConfiguration) {
return (
<StyledContainer id={constraintAccordionListId}>
<ConditionallyRender
condition={Boolean(showCreateButton && onAdd)}
show={
<div>
<StyledHelpIconBox>
<Typography>Constraints</Typography>
<HelpIcon
htmlTooltip
tooltip={
<Box>
<Typography variant='body2'>
Constraints are advanced
targeting rules that you can
use to enable a feature
toggle for a subset of your
users. Read more about
constraints{' '}
<a
href='https://docs.getunleash.io/reference/strategy-constraints'
target='_blank'
rel='noopener noreferrer'
>
here
</a>
</Typography>
</Box>
}
/>
</StyledHelpIconBox>
{constraints.map((constraint, index) => (
<Fragment key={objectId(constraint)}>
<ConditionallyRender
condition={index > 0}
show={
<StrategySeparator text='AND' />
}
/>
<ConstraintAccordion
constraint={constraint}
onEdit={onEdit?.bind(
null,
constraint,
)}
onCancel={onCancel.bind(
null,
index,
)}
onDelete={onRemove?.bind(
null,
index,
)}
onSave={onSave?.bind(null, index)}
editing={Boolean(
state.get(constraint)?.editing,
)}
compact
/>
</Fragment>
))}
<Button
sx={{ marginTop: '1rem' }}
type='button'
onClick={onAdd}
startIcon={<Add />}
variant='outlined'
color='primary'
data-testid='ADD_CONSTRAINT_BUTTON'
>
Add constraint
</Button>
</div>
}
/>
</StyledContainer>
);
}
return (
<StyledContainer id={constraintAccordionListId}>
<ConditionallyRender
@ -238,23 +129,12 @@ export const ConstraintAccordionList = forwardRef<
</StyledConstraintLabel>
}
/>
{constraints.map((constraint, index) => (
<Fragment key={objectId(constraint)}>
<ConditionallyRender
condition={index > 0}
show={<StrategySeparator text='AND' />}
/>
<ConstraintAccordion
constraint={constraint}
onEdit={onEdit?.bind(null, constraint)}
onCancel={onCancel.bind(null, index)}
onDelete={onRemove?.bind(null, index)}
onSave={onSave?.bind(null, index)}
editing={Boolean(state.get(constraint)?.editing)}
compact
/>
</Fragment>
))}
<ConstraintList
ref={ref}
setConstraints={setConstraints}
constraints={constraints}
state={state}
/>
<ConditionallyRender
condition={Boolean(showCreateButton && onAdd)}
show={
@ -292,3 +172,78 @@ export const ConstraintAccordionList = forwardRef<
);
},
);
interface IConstraintList {
constraints: IConstraint[];
setConstraints?: React.Dispatch<React.SetStateAction<IConstraint[]>>;
state: IUseWeakMap<IConstraint, IConstraintAccordionListItemState>;
}
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 (
<StyledContainer id={constraintAccordionListId}>
{constraints.map((constraint, index) => (
<Fragment key={objectId(constraint)}>
{console.log(state.get(constraint))}
<ConditionallyRender
condition={index > 0}
show={<StrategySeparator text='AND' />}
/>
<ConstraintAccordion
constraint={constraint}
onEdit={onEdit?.bind(null, constraint)}
onCancel={onCancel.bind(null, index)}
onDelete={onRemove?.bind(null, index)}
onSave={onSave?.bind(null, index)}
editing={Boolean(state.get(constraint)?.editing)}
compact
/>
</Fragment>
))}
</StyledContainer>
);
});

190
frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx Normal file
View File

@ -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<React.SetStateAction<IConstraint[]>>;
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<IConstraintAccordionListRef>,
);
const newStrategyConfiguration = useUiFlag('newStrategyConfiguration');
if (context.length === 0) {
return null;
}
if (newStrategyConfiguration) {
return (
<StyledContainer id={constraintAccordionListId}>
<ConditionallyRender
condition={Boolean(showCreateButton && onAdd)}
show={
<div>
<StyledHelpIconBox>
<Typography>Constraints</Typography>
<HelpIcon
htmlTooltip
tooltip={
<Box>
<Typography variant='body2'>
Constraints are advanced
targeting rules that you can
use to enable a feature
toggle for a subset of your
users. Read more about
constraints{' '}
<a
href='https://docs.getunleash.io/reference/strategy-constraints'
target='_blank'
rel='noopener noreferrer'
>
here
</a>
</Typography>
</Box>
}
/>
</StyledHelpIconBox>
<ConstraintList
ref={ref}
setConstraints={setConstraints}
constraints={constraints}
state={state}
/>
<Button
sx={{ marginTop: '1rem' }}
type='button'
onClick={onAdd}
startIcon={<Add />}
variant='outlined'
color='primary'
data-testid='ADD_CONSTRAINT_BUTTON'
>
Add constraint
</Button>
</div>
}
/>
</StyledContainer>
);
}
return (
<StyledContainer id={constraintAccordionListId}>
<ConditionallyRender
condition={
constraints && constraints.length > 0 && showLabel
}
show={
<StyledConstraintLabel>
Constraints
</StyledConstraintLabel>
}
/>
<ConstraintList
ref={ref}
setConstraints={setConstraints}
constraints={constraints}
state={state}
/>
<ConditionallyRender
condition={Boolean(showCreateButton && onAdd)}
show={
<div>
<StyledAddCustomLabel>
<p>Add any number of constraints</p>
<StyledHelpWrapper
title='View constraints documentation'
arrow
>
<a
href={
'https://docs.getunleash.io/reference/strategy-constraints'
}
target='_blank'
rel='noopener noreferrer'
>
<StyledHelp />
</a>
</StyledHelpWrapper>
</StyledAddCustomLabel>
<Button
type='button'
onClick={onAdd}
variant='outlined'
color='primary'
data-testid='ADD_CONSTRAINT_BUTTON'
>
Add constraint
</Button>
</div>
}
/>
</StyledContainer>
);
},
);

4
frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx
View File

@ -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 (
<ConstraintAccordionList
<FeatureStrategyConstraintAccordionList
constraints={constraints}
setConstraints={allowEditAndDelete ? setConstraints : undefined}
showCreateButton={showCreateButton}

2
frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
View File

@ -238,7 +238,7 @@ describe('NewFeatureStrategyCreate', () => {
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);

2
frontend/src/hooks/useWeakMap.ts
View File

@ -1,6 +1,6 @@
import { useState, useCallback, useRef, useMemo } from 'react';
interface IUseWeakMap<K, V> {
export interface IUseWeakMap<K, V> {
set: (k: K, v: V) => void;
get: (k: K) => V | undefined;
}