From 2e2bb9cf25efa67cc38e2d71a284b9c1321d1d17 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 6 Mar 2025 14:21:19 +0100 Subject: [PATCH] refactor: don't use absolute positioning for drag handle (#9434) Avoids absolutely positioning the drag handle by instead creating a two column grid where column 1 is the drag handle, column two is the milestone card. The grid has a negative margin based on the padding of the form container. I wanted to avoid modifying the form container component (because we use it in so many places), so I used css variables to store the information and hook into that further down the line. Rendered: Wide: ![image](https://github.com/user-attachments/assets/bb43b1b9-595b-475e-a59f-24ebf82df489) Narrow: ![image](https://github.com/user-attachments/assets/344b9c6f-08e7-43ca-8a02-1b224ccdd2c8) ## Known bugs and limitations The current drag implementation has some issues if you try to drag something over a large, expanded card. They'll trade places visually, but when you let go, the revert back to where they were. We can avoid that by modifying the onDrop function in the drag handler, but I don't want to do that before checking all the other places where we do drag and drop ([linear ticket](https://linear.app/unleash/issue/1-3458/drag-and-drop-is-a-little-finicky)). I also want to get UX to sign off on this before making those changes. --- .../common/FormTemplate/FormTemplate.tsx | 46 ++- .../MilestoneCard/MilestoneCard.tsx | 370 ++++++++++-------- 2 files changed, 228 insertions(+), 188 deletions(-) diff --git a/frontend/src/component/common/FormTemplate/FormTemplate.tsx b/frontend/src/component/common/FormTemplate/FormTemplate.tsx index 451f7d5823..b99da49a19 100644 --- a/frontend/src/component/common/FormTemplate/FormTemplate.tsx +++ b/frontend/src/component/common/FormTemplate/FormTemplate.tsx @@ -94,26 +94,38 @@ const StyledFormContent = styled('div', { return !['disablePadding', 'compactPadding'].includes(prop.toString()); }, })<{ disablePadding?: boolean; compactPadding?: boolean }>( - ({ theme, disablePadding, compactPadding }) => ({ - backgroundColor: theme.palette.background.paper, - display: 'flex', - flexDirection: 'column', - flexGrow: 1, - padding: disablePadding + ({ theme, disablePadding, compactPadding }) => { + const padding = disablePadding ? 0 : compactPadding ? theme.spacing(4) - : theme.spacing(6), - [theme.breakpoints.down('lg')]: { - padding: disablePadding ? 0 : theme.spacing(4), - }, - [theme.breakpoints.down(1100)]: { - width: '100%', - }, - [theme.breakpoints.down(500)]: { - padding: disablePadding ? 0 : theme.spacing(4, 2), - }, - }), + : theme.spacing(6); + + const paddingLgDown = disablePadding ? 0 : theme.spacing(4); + const padding500DownInline = disablePadding ? 0 : theme.spacing(2); + const padding500DownBlock = disablePadding ? 0 : theme.spacing(4); + + return { + '--form-content-padding': padding, + backgroundColor: theme.palette.background.paper, + display: 'flex', + flexDirection: 'column', + flexGrow: 1, + padding, + [theme.breakpoints.down('lg')]: { + padding: paddingLgDown, + '--form-content-padding': paddingLgDown, + }, + [theme.breakpoints.down(1100)]: { + width: '100%', + }, + [theme.breakpoints.down(500)]: { + paddingInline: padding500DownInline, + paddingBlock: padding500DownBlock, + '--form-content-padding': padding500DownInline, + }, + }; + }, ); const StyledFooter = styled('div')(({ theme }) => ({ diff --git a/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx b/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx index bc39899f0a..6c3512a8e1 100644 --- a/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx +++ b/frontend/src/component/releases/ReleasePlanTemplate/TemplateForm/MilestoneList/MilestoneCard/MilestoneCard.tsx @@ -27,13 +27,24 @@ import { StrategySeparator } from 'component/common/StrategySeparator/StrategySe import Edit from '@mui/icons-material/Edit'; import Delete from '@mui/icons-material/DeleteOutlined'; import { StrategyDraggableItem } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem'; +import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; const leftPadding = 3; +const DraggableCardContainer = styled('div')(({ theme }) => ({ + marginTop: theme.spacing(2), + '--left-padding': `var(--form-content-padding, ${theme.spacing(4)})`, + // for accessibility, never make button smaller than 32px + '--drag-column-width': `max(var(--left-padding), ${theme.spacing(4)})`, + '--left-offset': `calc(var(--left-padding) * -1)`, + marginLeft: `var(--left-offset)`, + display: 'grid', + gridTemplateColumns: `var(--drag-column-width) 1fr`, +})); + const StyledMilestoneCard = styled(Card, { shouldForwardProp: (prop) => prop !== 'hasError', })<{ hasError: boolean }>(({ theme, hasError }) => ({ - marginTop: theme.spacing(2), position: 'relative', overflow: 'initial', display: 'flex', @@ -61,7 +72,6 @@ const FlexContainer = styled('div')(({ theme }) => ({ const StyledAddStrategyButton = styled(Button)(({ theme }) => ({})); const StyledAccordion = styled(Accordion)(({ theme }) => ({ - marginTop: theme.spacing(2), boxShadow: 'none', background: 'none', display: 'flex', @@ -76,7 +86,7 @@ const StyledAccordion = styled(Accordion)(({ theme }) => ({ '&:before': { opacity: '0 !important', }, - '&.Mui-expanded': { marginTop: `${theme.spacing(2)} !important` }, + overflow: 'hidden', })); const StyledAccordionSummary = styled(AccordionSummary)(({ theme }) => ({ @@ -87,9 +97,8 @@ const StyledAccordionSummary = styled(AccordionSummary)(({ theme }) => ({ [theme.breakpoints.down(400)]: { padding: theme.spacing(1, 2), }, - '&.Mui-focusVisible': { - backgroundColor: theme.palette.background.paper, - padding: theme.spacing(0.5, 2, 0.3, 2), + '&:focus-visible': { + background: theme.palette.table.headerHover, }, })); @@ -106,7 +115,6 @@ const StyledAccordionFooter = styled('div')(({ theme }) => ({ justifyContent: 'flex-end', gap: theme.spacing(3), backgroundColor: 'inherit', - borderRadius: theme.shape.borderRadiusMedium, })); const StyledIconButton = styled(IconButton)(({ theme }) => ({ @@ -114,17 +122,27 @@ const StyledIconButton = styled(IconButton)(({ theme }) => ({ color: theme.palette.primary.main, })); -const StyledDragIcon = styled(IconButton)(({ theme }) => ({ +const DragButton = styled('button')(({ theme }) => ({ padding: 0, - position: 'absolute', cursor: 'grab', - left: theme.spacing(-4), - transition: 'color 0.2s ease-in-out', - '& > svg': { - color: 'action.active', + transition: 'background-color 0.2s ease-in-out', + backgroundColor: 'inherit', + border: 'none', + borderRadius: theme.shape.borderRadiusMedium, + color: theme.palette.text.secondary, + '&:hover, &:focus-visible': { + background: theme.palette.table.headerHover, + outline: 'none', }, })); +const DraggableContent = styled('span')(({ theme }) => ({ + paddingTop: theme.spacing(2.75), + display: 'block', + height: '100%', + width: '100%', +})); + export interface IMilestoneCardProps { milestone: IExtendedMilestonePayload; milestoneChanged: (milestone: IExtendedMilestonePayload) => void; @@ -169,9 +187,12 @@ export const MilestoneCard = ({ ); const dragHandle = ( - - - + + + + Drag to reorder + + ); const onClose = () => { @@ -342,64 +363,65 @@ export const MilestoneCard = ({ if (!milestone.strategies || milestone.strategies.length === 0) { return ( <> - + {dragHandle} - - - - - - - - - - - ({ - paddingBottom: theme.spacing(1), - }), - }} - > - { - openAddUpdateStrategyForm(strategy, false); - }} + + + - - - + + + + + + + ({ + paddingBottom: theme.spacing(1), + }), + }} + > + { + openAddUpdateStrategyForm( + strategy, + false, + ); + }} + /> + + + + {errors?.[milestone.id]} - { @@ -424,111 +446,117 @@ export const MilestoneCard = ({ return ( <> - setExpanded(change)} - > - - } - ref={dragItemRef} + + {dragHandle} + setExpanded(change)} > - {dragHandle} - - - - - {milestone.strategies.map((strg, index) => ( - - {index > 0 ? : null} - - - { - openAddUpdateStrategyForm( - strg, - true, - ); - }} - > - - - - milestoneStrategyDeleted( - strg.id, - ) - } - > - - - - } - /> - - ))} - - - - setAnchor(ev.currentTarget)} - > - Add strategy - - ({ - paddingBottom: theme.spacing(1), - }), - }} - > - { - openAddUpdateStrategyForm(strategy, false); - }} + - - - - + } + id={`milestone-accordion-summary-${milestone.id}`} + aria-controls={`milestone-accordion-details-${milestone.id}`} + > + + + + + {milestone.strategies.map((strg, index) => ( + + {index > 0 ? : null} + + + { + openAddUpdateStrategyForm( + strg, + true, + ); + }} + > + + + + milestoneStrategyDeleted( + strg.id, + ) + } + > + + + + } + /> + + ))} + + + + setAnchor(ev.currentTarget)} + > + Add strategy + + ({ + paddingBottom: theme.spacing(1), + }), + }} + > + { + openAddUpdateStrategyForm( + strategy, + false, + ); + }} + /> + + + + + {errors?.[milestone.id]}