change: [M3-8533, M3-8761] - Fix firewall rules table and Replace `re…

…act-beautiful-dnd` with `dnd-kit` lib (linode#11127) * Fix firewall rules table * Update the comment * Added changeset: Broken firewall rules table * Keeping PolicyRow to 2 grids for sm breakpoint * Add padding to headers and cells * Add line height to StyledHeaderItemBox * Update some styles * Update styles * Fix broken firewall rules table and replace react-beautiful-dnd with dnd-kit * Set cursor style to 'grab' or 'grabbing' based on drag state * isDragging state var pos fix * Add TouchSensor and collisionDetection * Add width to table cells for improved layout * Some cleanup... * Update width * Fix colors for pending deletion row and clean up styling for rule btn * Added changeset: Broken firewall rules table * Added changeset: Replace `react-beautiful-dnd` with `dnd-kit` library * Remove react-beautiful-dnd from the codebase * Remove react-beautiful-dnd from the codebase * Remove initial pos focus on drag end and enable rows to drag over others * Refactoring... * Update firewall e2e tests * Adjust label column to prevent text wrapping on mobile and some clean up... * Improve smoothness of row dragging * overflow logic & styling * add missing keyboard sensor * Fix weird scalling with varying row heights and enable drag on navs as well * Remove Label col nowrap to keep table inplace on overflow and some adjustments * revert overflow issue * revert overflow issue * fix sortable via keyboard behavior * Clean up imports * Fix scrolling behavior when using keyboard keys * Add changeset and fix typo * Fix focus reset when dragging elements via keyboard * Add theme compatible background color focus --------- Co-authored-by: Alban Bailly <abailly@akamai.com>
bnussman-akamai · Nov 18, 2024 · f3a9f37 · f3a9f37
1 parent bea1ccc
commit f3a9f37
Show file tree

Hide file tree

Showing 11 changed files with 569 additions and 299 deletions.
diff --git a/packages/manager/.changeset/pr-11127-changed-1729515938754.md b/packages/manager/.changeset/pr-11127-changed-1729515938754.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Changed
+---
+
+Replace `react-beautiful-dnd` with `dnd-kit` library ([#11127](https://github.com/linode/manager/pull/11127))
diff --git a/packages/manager/.changeset/pr-11127-fixed-1729515738287.md b/packages/manager/.changeset/pr-11127-fixed-1729515738287.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Fixed
+---
+
+Broken firewall rules table ([#11127](https://github.com/linode/manager/pull/11127))
diff --git a/packages/manager/.changeset/pr-11127-fixed-1730275015638.md b/packages/manager/.changeset/pr-11127-fixed-1730275015638.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Fixed
+---
+
+Table component styling issue for `noOverflow` property ([#11127](https://github.com/linode/manager/pull/11127))
diff --git a/packages/manager/cypress/e2e/core/firewalls/update-firewall.spec.ts b/packages/manager/cypress/e2e/core/firewalls/update-firewall.spec.ts
@@ -222,7 +222,7 @@ describe('update firewall', () => {
       // Confirm that the inbound rules are listed on edit page with expected configuration
       cy.findByText(inboundRule.label!)
         .should('be.visible')
-        .closest('li')
+        .closest('tr')
         .within(() => {
           cy.findByText(inboundRule.protocol).should('be.visible');
           cy.findByText(inboundRule.ports!).should('be.visible');
@@ -237,7 +237,7 @@ describe('update firewall', () => {
       // Confirm that the outbound rules are listed on edit page with expected configuration
       cy.findByText(outboundRule.label!)
         .should('be.visible')
-        .closest('li')
+        .closest('tr')
         .within(() => {
           cy.findByText(outboundRule.protocol).should('be.visible');
           cy.findByText(outboundRule.ports!).should('be.visible');

diff --git a/packages/manager/package.json b/packages/manager/package.json
@@ -14,6 +14,9 @@
     "url": "https://github.com/Linode/manager.git"
   },
   "dependencies": {
+    "@dnd-kit/core": "^6.1.0",
+    "@dnd-kit/sortable": "^8.0.0",
+    "@dnd-kit/utilities": "^3.2.2",
     "@emotion/react": "^11.11.1",
     "@emotion/styled": "^11.11.0",
     "@hookform/resolvers": "2.9.11",
@@ -61,7 +64,6 @@
     "qrcode.react": "^0.8.0",
     "ramda": "~0.25.0",
     "react": "^18.2.0",
-    "react-beautiful-dnd": "^13.0.0",
     "react-csv": "^2.0.3",
     "react-dom": "^18.2.0",
     "react-dropzone": "~11.2.0",

diff --git a/packages/manager/src/components/Table/Table.styles.ts b/packages/manager/src/components/Table/Table.styles.ts
@@ -13,25 +13,25 @@ export const StyledTableWrapper = styled('div', {
     'spacingTop',
   ]),
 })<TableProps>(({ theme, ...props }) => ({
+  '& thead': {
+    '& th': {
+      '&:first-of-type': {
+        borderLeft: 'none',
+      },
+      '&:last-of-type': {
+        borderRight: 'none',
+      },
+      backgroundColor: theme.bg.tableHeader,
+      borderBottom: `1px solid ${theme.borderColors.borderTable}`,
+      borderRight: `1px solid ${theme.borderColors.borderTable}`,
+      borderTop: `1px solid ${theme.borderColors.borderTable}`,
+      fontFamily: theme.font.bold,
+      padding: '10px 15px',
+    },
+  },
   marginBottom: props.spacingBottom !== undefined ? props.spacingBottom : 0,
   marginTop: props.spacingTop !== undefined ? props.spacingTop : 0,
   ...(!props.noOverflow && {
-    '& thead': {
-      '& th': {
-        '&:first-of-type': {
-          borderLeft: 'none',
-        },
-        '&:last-of-type': {
-          borderRight: 'none',
-        },
-        backgroundColor: theme.bg.tableHeader,
-        borderBottom: `1px solid ${theme.borderColors.borderTable}`,
-        borderRight: `1px solid ${theme.borderColors.borderTable}`,
-        borderTop: `1px solid ${theme.borderColors.borderTable}`,
-        fontFamily: theme.font.bold,
-        padding: '10px 15px',
-      },
-    },
     overflowX: 'auto',
     overflowY: 'hidden',
   }),

diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleTable.styles.ts b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleTable.styles.ts
@@ -7,47 +7,29 @@ import type { FirewallRuleTableRowProps } from './FirewallRuleTable';
 
 type StyledFirewallRuleButtonProps = Pick<FirewallRuleTableRowProps, 'status'>;
 
-interface FirewallRuleTableRowPropsWithRuleId
+interface FirewallRuleTableRowPropsWithRuleIndex
   extends Pick<FirewallRuleTableRowProps, 'disabled' | 'originalIndex'> {
-  ruleId: number;
+  ruleIndex: number;
 }
 
-interface StyledFirewallRuleBoxProps
-  extends FirewallRuleTableRowPropsWithRuleId {
+interface StyledFirewallRuleTableRowProps
+  extends FirewallRuleTableRowPropsWithRuleIndex {
   status: FirewallRuleTableRowProps['status'];
 }
 
-export const sxBox = {
-  alignItems: 'center',
-  display: 'flex',
-  width: '100%',
-};
-
-export const sxItemSpacing = {
-  padding: `0 8px`,
-};
-
-export const StyledFirewallRuleBox = styled(Box, {
-  label: 'StyledFirewallRuleBox',
-  shouldForwardProp: omittedProps(['originalIndex', 'ruleId']),
-})<StyledFirewallRuleBoxProps>(
-  ({ disabled, originalIndex, ruleId, status, theme }) => ({
-    borderBottom: `1px solid ${theme.borderColors.borderTable}`,
-    color: theme.textColors.tableStatic,
-    fontSize: '0.875rem',
-    margin: 0,
-    ...sxBox,
-
+// Note: Use 'tr' instead of 'TableRow' here for a smoother draggable user experience.
+export const StyledTableRow = styled('tr', {
+  label: 'StyledTableRow',
+  shouldForwardProp: omittedProps(['originalIndex', 'ruleIndex']),
+})<StyledFirewallRuleTableRowProps>(
+  ({ disabled, originalIndex, ruleIndex, status, theme }) => ({
     // Conditional styles
-    // Highlight the row if it's been modified or reordered. ID is the current index,
+    // Highlight the row if it's been modified or reordered. ruleIndex is the current index,
     // so if it doesn't match the original index we know that the rule has been moved.
     ...(status === 'PENDING_DELETION' || disabled
-      ? {
-          '& td': { color: '#D2D3D4' },
-          backgroundColor: 'rgba(247, 247, 247, 0.25)',
-        }
+      ? { backgroundColor: theme.color.grey7 }
       : {}),
-    ...(status === 'MODIFIED' || status === 'NEW' || originalIndex !== ruleId
+    ...(status === 'MODIFIED' || status === 'NEW' || originalIndex !== ruleIndex
       ? { backgroundColor: theme.bg.lightBlue1 }
       : {}),
     ...(status === 'NOT_MODIFIED' ? { backgroundColor: theme.bg.bgPaper } : {}),
@@ -57,38 +39,17 @@ export const StyledFirewallRuleBox = styled(Box, {
 export const StyledInnerBox = styled(Box, { label: 'StyledInnerBox' })(
   ({ theme }) => ({
     backgroundColor: theme.bg.tableHeader,
-    color: theme.textColors.tableHeader,
     fontFamily: theme.font.bold,
     fontSize: '.875rem',
-    height: '46px',
-  })
-);
-
-export const StyledUlBox = styled(Box, { label: 'StyledUlBox' })(
-  ({ theme }) => ({
-    alignItems: 'center',
-    backgroundColor: theme.bg.bgPaper,
-    borderBottom: `1px solid ${theme.borderColors.borderTable}`,
-    color: theme.textColors.tableStatic,
-    display: 'flex',
-    fontSize: '0.875rem',
-    justifyContent: 'center',
-    padding: theme.spacing(1),
-    width: '100%',
   })
 );
 
 export const StyledFirewallRuleButton = styled('button', {
   label: 'StyledFirewallRuleButton',
-})<StyledFirewallRuleButtonProps>(({ status, theme }) => ({
+})<StyledFirewallRuleButtonProps>(() => ({
   backgroundColor: 'transparent',
   border: 'none',
   cursor: 'pointer',
-
-  // Conditional styles
-  ...(status !== 'PENDING_DELETION'
-    ? { backgroundColor: theme.bg.lightBlue1 }
-    : {}),
 }));
 
 export const StyledFirewallTableButton = styled(Button, {
@@ -137,11 +98,3 @@ export const StyledDragIndicator = styled(DragIndicator, {
   position: 'relative',
   top: 2,
 }));
-
-export const StyledUl = styled('ul', { label: 'StyledUl' })(({ theme }) => ({
-  backgroundColor: theme.color.border3,
-  listStyle: 'none',
-  margin: 0,
-  paddingLeft: 0,
-  width: '100%',
-}));
diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleTable.test.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleTable.test.tsx
@@ -1,5 +1,6 @@
 import { firewallRuleToRowData } from './FirewallRuleTable';
-import { ExtendedFirewallRule } from './firewallRuleEditor';
+
+import type { ExtendedFirewallRule } from './firewallRuleEditor';
 
 describe('Firewall rule table tests', () => {
   describe('firewallRuleToRowData', () => {