From 220550071fcb1f8ad7f915ece18fb2f5edba7d42 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 7 Mar 2025 13:09:36 +0100 Subject: [PATCH] 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) --- .../StrategyItemContainer.test.tsx | 2 +- .../StrategyItemContainer.tsx | 83 ++++++----- .../EnvironmentAccordionBody.tsx | 134 ++++++++++-------- .../StrategyItem/StrategyItem.tsx | 3 + .../ReleasePlanMilestone.tsx | 1 + 5 files changed, 126 insertions(+), 97 deletions(-) diff --git a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.test.tsx b/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.test.tsx index 902dc10af6..6043782183 100644 --- a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.test.tsx +++ b/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.test.tsx @@ -42,7 +42,7 @@ test('should render strategy name, custom title and description', async () => { />, ); - expect(screen.getByText('strategy name')).toBeInTheDocument(); + expect(screen.getByText('strategy name:')).toBeInTheDocument(); expect(screen.getByText('description')).toBeInTheDocument(); await screen.findByText('custom title'); // behind async flag }); diff --git a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx b/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx index 1f5bf69134..d908984777 100644 --- a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx +++ b/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx @@ -1,16 +1,16 @@ import type React from 'react'; import type { DragEventHandler, FC, ReactNode } from 'react'; import DragIndicator from '@mui/icons-material/DragIndicator'; -import { Box, IconButton, styled } from '@mui/material'; +import { Box, IconButton, Typography, styled } from '@mui/material'; import type { IFeatureStrategy } from 'interfaces/strategy'; import { formatStrategyName } from 'utils/strategyNames'; -import StringTruncator from 'component/common/StringTruncator/StringTruncator'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import type { PlaygroundStrategySchema } from 'openapi'; import { Badge } from '../Badge/Badge'; import { Link } from 'react-router-dom'; type StrategyItemContainerProps = { + strategyHeaderLevel?: 1 | 2 | 3 | 4 | 5 | 6; strategy: IFeatureStrategy | PlaygroundStrategySchema; onDragStart?: DragEventHandler; onDragEnd?: DragEventHandler; @@ -44,15 +44,17 @@ const StyledCustomTitle = styled('div')(({ theme }) => ({ display: 'block', }, })); -const StyledHeaderContainer = styled('div')({ - flexDirection: 'column', - justifyContent: 'center', - verticalAlign: 'middle', -}); +const StyledHeaderContainer = styled('hgroup')(({ theme }) => ({ + display: 'flex', + flexFlow: 'row', + columnGap: '1ch', + fontSize: theme.typography.body1.fontSize, + '.strategy-name': { + fontWeight: 'bold', + }, +})); -const NewStyledContainer = styled(Box, { - shouldForwardProp: (prop) => prop !== 'disabled', -})({ +const StyledContainer = styled('article')({ background: 'inherit', }); @@ -64,7 +66,6 @@ const NewStyledHeader = styled('div', { display: 'flex', gap: theme.spacing(1), alignItems: 'center', - fontWeight: theme.typography.fontWeightMedium, paddingLeft: draggable ? theme.spacing(1) : theme.spacing(2), color: disabled ? theme.palette.text.secondary @@ -77,18 +78,19 @@ export const StrategyItemContainer: FC = ({ onDragStart, onDragEnd, headerItemsRight, + strategyHeaderLevel = 3, children, style = {}, description, }) => { const StrategyHeaderLink: React.FC<{ children?: React.ReactNode }> = - 'links' in strategy + 'links' in strategy // todo: revisit this when we get to playground, related to flag `flagOverviewRedesign` ? ({ children }) => {children} : ({ children }) => <> {children} ; return ( - + = ({ )} /> - - - - + + + {strategy.title ? ( + <> +

{formatStrategyName( - String(strategy.title), + String(strategy.name), )} - + : +

+ + {strategy.title} + + + ) : ( + + {formatStrategyName(String(strategy.name))} + + )} + + {description} + } /> - - - {description} - - } - /> - + + = ({ {children} - + ); }; diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/EnvironmentAccordionBody.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/EnvironmentAccordionBody.tsx index db4665ad96..459ae04cb8 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/EnvironmentAccordionBody.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/EnvironmentAccordionBody.tsx @@ -36,11 +36,25 @@ const StyledAccordionBodyInnerContainer = styled('div')(({ theme }) => ({ }, })); -export const StyledContentList = styled('ol')({ +export const StyledContentList = styled('ol')(({ theme }) => ({ listStyle: 'none', padding: 0, margin: 0, -}); + + '& > li': { + paddingBlock: theme.spacing(2.5), + position: 'relative', + }, + '&:not(li > &) > li:first-of-type': { + // select first list elements in lists that are not directly nested + // within other lists. + paddingTop: theme.spacing(1), + }, + '& > li:has(> ol)': { + // nested lists add their own padding to their list items, so we don't want to double it up. + paddingBlock: 0, + }, +})); export const StyledListItem = styled('li', { shouldForwardProp: (prop) => prop !== 'type', @@ -50,11 +64,6 @@ export const StyledListItem = styled('li', { type === 'release plan' ? theme.palette.background.elevation2 : theme.palette.background.elevation1, - position: 'relative', - paddingBlock: theme.spacing(2.5), - '&:first-of-type': { - paddingTop: theme.spacing(1), - }, })); const PaginatedStrategyContainer = styled('div')(({ theme }) => ({ @@ -243,75 +252,82 @@ export const EnvironmentAccordionBody = ({ /> ))} - {strategies.length < 50 || !manyStrategiesPagination ? ( - - {strategies.map((strategy, index) => ( - - {index > 0 || - releasePlans.length > 0 ? ( - - ) : null} - - - - ))} - - ) : ( - - - We noticed you're using a high number of - activation strategies. To ensure a more - targeted approach, consider leveraging - constraints or segments. - +
  • + {releasePlans.length > 0 ? ( + + ) : null} + {strategies.length < 50 || + !manyStrategiesPagination ? ( - {page.map((strategy, index) => ( + {strategies.map((strategy, index) => ( - {index > 0 || - releasePlans.length > 0 ? ( + {index > 0 ? ( ) : null} ))} - - setPageIndex(page - 1) - } - /> - - )} + ) : ( + + + We noticed you're using a high number of + activation strategies. To ensure a more + targeted approach, consider leveraging + constraints or segments. + + + {page.map((strategy, index) => ( + + {index > 0 ? ( + + ) : null} + + + + ))} + + + setPageIndex(page - 1) + } + /> + + )} +
    • ) : ( ; onDragEnd?: DragEventHandler; + strategyHeaderLevel?: 1 | 2 | 3 | 4 | 5 | 6; }; export const StrategyItem: FC = ({ @@ -17,9 +18,11 @@ export const StrategyItem: FC = ({ onDragStart, onDragEnd, headerItemsRight, + strategyHeaderLevel, }) => { return ( 0 ? : null}