Skip to content

Commit 2205500

Browse files
thomasheartmanthomasheartman
authored
chore(1-3450): Place strategy names and titles on the same line (and fix list nesting issues) (#9443)
Moves strategy titles and names onto the same line, as per the new designs. In doing so, I've also updated the component to use a more semantic hgroup with the header being the strategy title if it exists or the strategy name if not. The downside of being more semantically correct here is that we need to know what header level we want the strategy to use. In most cases, that's 3 (e.g. flag name > environment > strategy, release plan > milestone > strategy), but for plans on flag envs, it's 4 (flag name > env > milestone name > strategy). I've also taken the opportunity to fix a little mistake I made earlier. `ol`s can only have `li` children, and I'd forgotten to wrap a nested `ol` inside an `li`. The changes in `EnvironmentAccordionBody` all relate to that change. Because we now have several layers of lists nested within each other, dealing with styling and padding gets a little tricky, but CSS has the power do help us out here. Rendered: ![image](https://github.com/user-attachments/assets/634615fe-b06d-4baa-8aa3-22447d1fb8b4)
1 parent f698790 commit 2205500

File tree

5 files changed

+126
-97
lines changed

5 files changed

+126
-97
lines changed

‎frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ test('should render strategy name, custom title and description', async () => {
4242
/>,
4343
);
4444

45-
expect(screen.getByText('strategy name')).toBeInTheDocument();
45+
expect(screen.getByText('strategy name:')).toBeInTheDocument();
4646
expect(screen.getByText('description')).toBeInTheDocument();
4747
await screen.findByText('custom title'); // behind async flag
4848
});

‎frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
import type React from 'react';
22
import type { DragEventHandler, FC, ReactNode } from 'react';
33
import DragIndicator from '@mui/icons-material/DragIndicator';
4-
import { Box, IconButton, styled } from '@mui/material';
4+
import { Box, IconButton, Typography, styled } from '@mui/material';
55
import type { IFeatureStrategy } from 'interfaces/strategy';
66
import { formatStrategyName } from 'utils/strategyNames';
7-
import StringTruncator from 'component/common/StringTruncator/StringTruncator';
87
import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
98
import type { PlaygroundStrategySchema } from 'openapi';
109
import { Badge } from '../Badge/Badge';
1110
import { Link } from 'react-router-dom';
1211

1312
type StrategyItemContainerProps = {
13+
strategyHeaderLevel?: 1 | 2 | 3 | 4 | 5 | 6;
1414
strategy: IFeatureStrategy | PlaygroundStrategySchema;
1515
onDragStart?: DragEventHandler<HTMLButtonElement>;
1616
onDragEnd?: DragEventHandler<HTMLButtonElement>;
@@ -44,15 +44,17 @@ const StyledCustomTitle = styled('div')(({ theme }) => ({
4444
display: 'block',
4545
},
4646
}));
47-
const StyledHeaderContainer = styled('div')({
48-
flexDirection: 'column',
49-
justifyContent: 'center',
50-
verticalAlign: 'middle',
51-
});
47+
const StyledHeaderContainer = styled('hgroup')(({ theme }) => ({
48+
display: 'flex',
49+
flexFlow: 'row',
50+
columnGap: '1ch',
51+
fontSize: theme.typography.body1.fontSize,
52+
'.strategy-name': {
53+
fontWeight: 'bold',
54+
},
55+
}));
5256

53-
const NewStyledContainer = styled(Box, {
54-
shouldForwardProp: (prop) => prop !== 'disabled',
55-
})({
57+
const StyledContainer = styled('article')({
5658
background: 'inherit',
5759
});
5860

@@ -64,7 +66,6 @@ const NewStyledHeader = styled('div', {
6466
display: 'flex',
6567
gap: theme.spacing(1),
6668
alignItems: 'center',
67-
fontWeight: theme.typography.fontWeightMedium,
6869
paddingLeft: draggable ? theme.spacing(1) : theme.spacing(2),
6970
color: disabled
7071
? theme.palette.text.secondary
@@ -77,18 +78,19 @@ export const StrategyItemContainer: FC<StrategyItemContainerProps> = ({
7778
onDragStart,
7879
onDragEnd,
7980
headerItemsRight,
81+
strategyHeaderLevel = 3,
8082
children,
8183
style = {},
8284
description,
8385
}) => {
8486
const StrategyHeaderLink: React.FC<{ children?: React.ReactNode }> =
85-
'links' in strategy
87+
'links' in strategy // todo: revisit this when we get to playground, related to flag `flagOverviewRedesign`
8688
? ({ children }) => <Link to={strategy.links.edit}>{children}</Link>
8789
: ({ children }) => <> {children} </>;
8890

8991
return (
9092
<Box sx={{ position: 'relative' }}>
91-
<NewStyledContainer style={style}>
93+
<StyledContainer style={style}>
9294
<NewStyledHeader
9395
draggable={Boolean(onDragStart)}
9496
disabled={Boolean(strategy?.disabled)}
@@ -112,33 +114,40 @@ export const StrategyItemContainer: FC<StrategyItemContainerProps> = ({
112114
</DragIcon>
113115
)}
114116
/>
115-
<StyledHeaderContainer>
116-
<StrategyHeaderLink>
117-
<StringTruncator
118-
maxWidth='400'
119-
maxLength={15}
120-
text={formatStrategyName(String(strategy.name))}
121-
/>
122-
<ConditionallyRender
123-
condition={Boolean(strategy.title)}
124-
show={
125-
<StyledCustomTitle>
117+
<StrategyHeaderLink>
118+
<StyledHeaderContainer>
119+
{strategy.title ? (
120+
<>
121+
<p className='strategy-name'>
126122
{formatStrategyName(
127-
String(strategy.title),
123+
String(strategy.name),
128124
)}
129-
</StyledCustomTitle>
125+
:
126+
</p>
127+
<Typography
128+
component={`h${strategyHeaderLevel}`}
129+
>
130+
{strategy.title}
131+
</Typography>
132+
</>
133+
) : (
134+
<Typography
135+
className='strategy-name'
136+
component={`h${strategyHeaderLevel}`}
137+
>
138+
{formatStrategyName(String(strategy.name))}
139+
</Typography>
140+
)}
141+
<ConditionallyRender
142+
condition={Boolean(description)}
143+
show={
144+
<StyledDescription>
145+
{description}
146+
</StyledDescription>
130147
}
131148
/>
132-
</StrategyHeaderLink>
133-
<ConditionallyRender
134-
condition={Boolean(description)}
135-
show={
136-
<StyledDescription>
137-
{description}
138-
</StyledDescription>
139-
}
140-
/>
141-
</StyledHeaderContainer>
149+
</StyledHeaderContainer>
150+
</StrategyHeaderLink>
142151

143152
<ConditionallyRender
144153
condition={Boolean(strategy?.disabled)}
@@ -160,7 +169,7 @@ export const StrategyItemContainer: FC<StrategyItemContainerProps> = ({
160169
</Box>
161170
</NewStyledHeader>
162171
<Box sx={{ p: 2, pt: 0 }}>{children}</Box>
163-
</NewStyledContainer>
172+
</StyledContainer>
164173
</Box>
165174
);
166175
};

‎frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/EnvironmentAccordionBody.tsx

Lines changed: 75 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,25 @@ const StyledAccordionBodyInnerContainer = styled('div')(({ theme }) => ({
3636
},
3737
}));
3838

39-
export const StyledContentList = styled('ol')({
39+
export const StyledContentList = styled('ol')(({ theme }) => ({
4040
listStyle: 'none',
4141
padding: 0,
4242
margin: 0,
43-
});
43+
44+
'& > li': {
45+
paddingBlock: theme.spacing(2.5),
46+
position: 'relative',
47+
},
48+
'&:not(li > &) > li:first-of-type': {
49+
// select first list elements in lists that are not directly nested
50+
// within other lists.
51+
paddingTop: theme.spacing(1),
52+
},
53+
'& > li:has(> ol)': {
54+
// nested lists add their own padding to their list items, so we don't want to double it up.
55+
paddingBlock: 0,
56+
},
57+
}));
4458

4559
export const StyledListItem = styled('li', {
4660
shouldForwardProp: (prop) => prop !== 'type',
@@ -50,11 +64,6 @@ export const StyledListItem = styled('li', {
5064
type === 'release plan'
5165
? theme.palette.background.elevation2
5266
: theme.palette.background.elevation1,
53-
position: 'relative',
54-
paddingBlock: theme.spacing(2.5),
55-
'&:first-of-type': {
56-
paddingTop: theme.spacing(1),
57-
},
5867
}));
5968

6069
const PaginatedStrategyContainer = styled('div')(({ theme }) => ({
@@ -243,75 +252,82 @@ export const EnvironmentAccordionBody = ({
243252
/>
244253
</StyledListItem>
245254
))}
246-
{strategies.length < 50 || !manyStrategiesPagination ? (
247-
<StyledContentList>
248-
{strategies.map((strategy, index) => (
249-
<StyledListItem key={strategy.id}>
250-
{index > 0 ||
251-
releasePlans.length > 0 ? (
252-
<StrategySeparator />
253-
) : null}
254-
255-
<ProjectEnvironmentStrategyDraggableItem
256-
strategy={strategy}
257-
index={index}
258-
environmentName={
259-
featureEnvironment.name
260-
}
261-
otherEnvironments={
262-
otherEnvironments
263-
}
264-
isDragging={
265-
dragItem?.id === strategy.id
266-
}
267-
onDragStartRef={onDragStartRef}
268-
onDragOver={onDragOver(strategy.id)}
269-
onDragEnd={onDragEnd}
270-
/>
271-
</StyledListItem>
272-
))}
273-
</StyledContentList>
274-
) : (
275-
<PaginatedStrategyContainer>
276-
<StyledAlert severity='warning'>
277-
We noticed you're using a high number of
278-
activation strategies. To ensure a more
279-
targeted approach, consider leveraging
280-
constraints or segments.
281-
</StyledAlert>
255+
<li>
256+
{releasePlans.length > 0 ? (
257+
<StrategySeparator />
258+
) : null}
259+
{strategies.length < 50 ||
260+
!manyStrategiesPagination ? (
282261
<StyledContentList>
283-
{page.map((strategy, index) => (
262+
{strategies.map((strategy, index) => (
284263
<StyledListItem key={strategy.id}>
285-
{index > 0 ||
286-
releasePlans.length > 0 ? (
264+
{index > 0 ? (
287265
<StrategySeparator />
288266
) : null}
289267

290268
<ProjectEnvironmentStrategyDraggableItem
291269
strategy={strategy}
292-
index={
293-
index + pageIndex * pageSize
294-
}
270+
index={index}
295271
environmentName={
296272
featureEnvironment.name
297273
}
298274
otherEnvironments={
299275
otherEnvironments
300276
}
277+
isDragging={
278+
dragItem?.id === strategy.id
279+
}
280+
onDragStartRef={onDragStartRef}
281+
onDragOver={onDragOver(
282+
strategy.id,
283+
)}
284+
onDragEnd={onDragEnd}
301285
/>
302286
</StyledListItem>
303287
))}
304288
</StyledContentList>
305-
<Pagination
306-
count={pages.length}
307-
shape='rounded'
308-
page={pageIndex + 1}
309-
onChange={(_, page) =>
310-
setPageIndex(page - 1)
311-
}
312-
/>
313-
</PaginatedStrategyContainer>
314-
)}
289+
) : (
290+
<PaginatedStrategyContainer>
291+
<StyledAlert severity='warning'>
292+
We noticed you're using a high number of
293+
activation strategies. To ensure a more
294+
targeted approach, consider leveraging
295+
constraints or segments.
296+
</StyledAlert>
297+
<StyledContentList>
298+
{page.map((strategy, index) => (
299+
<StyledListItem key={strategy.id}>
300+
{index > 0 ? (
301+
<StrategySeparator />
302+
) : null}
303+
304+
<ProjectEnvironmentStrategyDraggableItem
305+
strategy={strategy}
306+
index={
307+
index +
308+
pageIndex * pageSize
309+
}
310+
environmentName={
311+
featureEnvironment.name
312+
}
313+
otherEnvironments={
314+
otherEnvironments
315+
}
316+
/>
317+
</StyledListItem>
318+
))}
319+
</StyledContentList>
320+
<Pagination
321+
count={pages.length}
322+
shape='rounded'
323+
page={pageIndex + 1}
324+
onChange={(_, page) =>
325+
setPageIndex(page - 1)
326+
}
327+
/>
328+
</PaginatedStrategyContainer>
329+
)}
330+
</li>
315331
</StyledContentList>
316332
) : (
317333
<FeatureStrategyEmpty

‎frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyItem.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,19 @@ type StrategyItemProps = {
1010
strategy: IFeatureStrategy;
1111
onDragStart?: DragEventHandler<HTMLButtonElement>;
1212
onDragEnd?: DragEventHandler<HTMLButtonElement>;
13+
strategyHeaderLevel?: 1 | 2 | 3 | 4 | 5 | 6;
1314
};
1415

1516
export const StrategyItem: FC<StrategyItemProps> = ({
1617
strategy,
1718
onDragStart,
1819
onDragEnd,
1920
headerItemsRight,
21+
strategyHeaderLevel,
2022
}) => {
2123
return (
2224
<NewStrategyItemContainer
25+
strategyHeaderLevel={strategyHeaderLevel}
2326
strategy={strategy}
2427
onDragStart={onDragStart}
2528
onDragEnd={onDragEnd}

‎frontend/src/component/feature/FeatureView/FeatureOverview/ReleasePlan/ReleasePlanMilestone/ReleasePlanMilestone.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ export const ReleasePlanMilestone = ({
123123
{index > 0 ? <StrategySeparator /> : null}
124124

125125
<StrategyItem
126+
strategyHeaderLevel={4}
126127
strategy={{
127128
...strategy,
128129
name:

0 commit comments

Comments
 (0)