fix: adapt container padding to grid mode (#1140)

* fix: add padding to container and adapt width calc * test: misspelling in unit test * refactor: and padding and adjust widths * refactor: clean up duplicate code * test: fix * fix: adjust padding instead of size calc for column case * fix: rm border too * fix: adjust into perfection * test: update test image for grid * fix: rm border * test: upload new img
qlik-oss · Mar 14, 2023 · fe78bac · fe78bac
1 parent e2a43c0
commit fe78bac
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 63 deletions.
diff --git a/apis/nucleus/src/components/listbox/ListBoxInline.jsx b/apis/nucleus/src/components/listbox/ListBoxInline.jsx
@@ -24,22 +24,28 @@ const drillDownIconWidth = 24;
 const classes = {
   listBoxHeader: `${PREFIX}-listBoxHeader`,
   screenReaderOnly: `${PREFIX}-screenReaderOnly`,
+  listboxWrapper: `${PREFIX}-listboxWrapper`,
 };
 
-const StyledGrid = styled(Grid)(({ theme }) => ({
-  backgroundColor: theme.listBox?.backgroundColor ?? theme.palette.background.default,
-  [`& .${classes.listBoxHeader}`]: {
-    alignSelf: 'center',
-    display: 'inline-flex',
-    width: `calc(100% - ${searchIconWidth}px)`,
-  },
-  [`& .${classes.screenReaderOnly}`]: {
-    position: 'absolute',
-    height: 0,
-    width: 0,
-    overflow: 'hidden',
-  },
-}));
+const StyledGrid = styled(Grid, { shouldForwardProp: (p) => !['containerPadding'].includes(p) })(
+  ({ theme, containerPadding }) => ({
+    backgroundColor: theme.listBox?.backgroundColor ?? theme.palette.background.default,
+    [`& .${classes.listBoxHeader}`]: {
+      alignSelf: 'center',
+      display: 'inline-flex',
+      width: `calc(100% - ${searchIconWidth}px)`,
+    },
+    [`& .${classes.screenReaderOnly}`]: {
+      position: 'absolute',
+      height: 0,
+      width: 0,
+      overflow: 'hidden',
+    },
+    [`& .${classes.listboxWrapper}`]: {
+      padding: containerPadding,
+    },
+  })
+);
 
 const Title = styled(Typography)(({ theme }) => ({
   color: theme.listBox?.title?.main?.color,
@@ -185,7 +191,8 @@ function ListBoxInline({ options, layout }) {
   const showTitle = true;
   const showSearchToggle = search === 'toggle' && showSearch;
   const searchVisible = (search === true || showSearchToggle) && !selectDisabled();
-  const dense = layout.layoutOptions?.dense ?? false;
+  const { layoutOptions = {} } = layout;
+  const dense = layoutOptions.dense ?? false;
   const searchHeight = dense ? 27 : 40;
   const extraheight = dense ? 39 : 49;
   const minHeight = 49 + (searchVisible ? searchHeight : 0) + extraheight;
@@ -205,14 +212,26 @@ function ListBoxInline({ options, layout }) {
   const drillDownPaddingLeft = showSearchOrLockIcon ? 0 : ICON_PADDING;
   const headerPaddingLeft = CELL_PADDING_LEFT - (showIcons ? ICON_PADDING : 0);
 
+  // Add a container padding for grid mode to harmonize with the grid item margins (should sum to 8px).
+  const isGridMode = layoutOptions?.dataLayout === 'grid';
+  let containerPadding;
+  if (isGridMode) {
+    containerPadding = layoutOptions.layoutOrder === 'row' ? '2px 4px' : '2px 6px 2px 4px';
+  }
+
   return (
     <StyledGrid
       className="listbox-container"
       container
       tabIndex={keyboard.enabled && !keyboard.active ? 0 : -1}
       direction="column"
       gap={0}
-      style={{ height: '100%', minHeight: `${minHeight}px`, flexFlow: 'column nowrap' }}
+      containerPadding={containerPadding}
+      style={{
+        height: '100%',
+        minHeight: `${minHeight}px`,
+        flexFlow: 'column nowrap',
+      }}
       onKeyDown={handleKeyDown}
       onMouseEnter={handleOnMouseEnter}
       onMouseLeave={handleOnMouseLeave}
@@ -309,7 +328,7 @@ function ListBoxInline({ options, layout }) {
             searchEnabled={searchEnabled}
           />
         </Grid>
-        <Grid item xs>
+        <Grid item xs className={classes.listboxWrapper}>
           <div className={classes.screenReaderOnly}>{translator.get('Listbox.ScreenReaderInstructions')}</div>
           <AutoSizer>
             {({ height, width }) => (

diff --git a/apis/nucleus/src/components/listbox/assets/__tests__/get-list-sizes.test.jsx b/apis/nucleus/src/components/listbox/assets/__tests__/get-list-sizes.test.jsx
@@ -42,15 +42,15 @@ describe('get-list-sizes', () => {
     const sizes = getListSizes(args);
     expect(sizes).toEqual({
       columnCount: 1,
-      columnWidth: 47.5,
+      columnWidth: 49.5,
       count: 200,
       itemPadding: 4,
       itemSize: 29,
       listCount: 100,
       listHeight: 300,
       frequencyWidth: 40,
       maxCount: {
-        column: 706315,
+        column: 677777,
         row: 577000,
       },
       overflowStyling: {
@@ -134,7 +134,7 @@ describe('get-list-sizes', () => {
     });
   });
 
-  it('A minimum item width should kick in if text is short and reverve extra space for frequency', () => {
+  it('A minimum item width should kick in if text is short and reserve extra space for frequency', () => {
     args.layout.layoutOptions.dataLayout = 'grid';
     args.layout.layoutOptions.layoutOrder = 'column';
     args.textWidth = 10;
@@ -176,15 +176,15 @@ describe('get-list-sizes', () => {
     const sizes = getListSizes(args);
     expect(sizes).toEqual({
       columnCount: 1,
-      columnWidth: 47.5,
+      columnWidth: 49.5,
       count: 200,
       itemPadding: 4,
       itemSize: 29,
       listCount: args.listCount,
       listHeight: 300,
       frequencyWidth: 40,
       maxCount: {
-        column: 706315,
+        column: 677777,
         row: 577000,
       },
       overflowStyling: {

diff --git a/apis/nucleus/src/components/listbox/assets/get-list-sizes.js b/apis/nucleus/src/components/listbox/assets/get-list-sizes.js
@@ -35,17 +35,21 @@ export default function getListSizes({ layout, width, height, listCount, count,
   const listHeight = height ?? 8 * itemSize;
 
   if (layoutOrder) {
+    // Modify container width to achieve the exact design with 8px margins on each side (left and right).
+    let containerWidth = width;
+
     if (layoutOrder === 'row') {
+      containerWidth += itemPadding * 2;
       overflowStyling = { overflowX: 'hidden' };
       const maxColumns = maxVisibleColumns?.maxColumns || 3;
 
       if (maxVisibleColumns?.auto !== false) {
-        columnCount = Math.min(listCount, Math.ceil((width - scrollBarWidth) / columnAutoWidth)); // TODO: smarter sizing... based on glyph count + font size etc...??
+        columnCount = Math.min(listCount, Math.ceil((containerWidth - scrollBarWidth) / columnAutoWidth)); // TODO: smarter sizing... based on glyph count + font size etc...??
       } else {
         columnCount = Math.min(listCount, maxColumns);
       }
       rowCount = Math.ceil(listCount / columnCount);
-      columnWidth = (width - scrollBarWidth) / columnCount;
+      columnWidth = (containerWidth - scrollBarWidth) / columnCount;
     } else {
       overflowStyling = { overflowY: 'hidden' };
       const maxRows = maxVisibleRows?.maxRows || 3;
@@ -57,7 +61,7 @@ export default function getListSizes({ layout, width, height, listCount, count,
       }
 
       columnCount = Math.ceil(listCount / rowCount);
-      columnWidth = Math.max(columnAutoWidth, width / columnCount);
+      columnWidth = Math.max(columnAutoWidth, containerWidth / columnCount);
     }
   }
 

diff --git a/...x/components/grid-list-components/__tests__/__snapshots__/styled-components.spec.jsx.snap b/...x/components/grid-list-components/__tests__/__snapshots__/styled-components.spec.jsx.snap
@@ -18,7 +18,7 @@ exports[`styled-components StyledFixedSizeGrid should return a renderable base c
 
 exports[`styled-components StyledFixedSizeGrid should return a renderable base component for Grid 2`] = `
 {
-  "css-qk0o61": "&.ListBox-styledScrollbars{scrollbar-color:#BBB #f1f1f1;&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}}&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}",
+  "css-1nwu5vb": "&.ListBox-styledScrollbars{scrollbar-color:#BBB #f1f1f1;&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}}",
 }
 `;
 
@@ -39,6 +39,6 @@ exports[`styled-components StyledFixedSizeList should return a renderable base c
 
 exports[`styled-components StyledFixedSizeList should return a renderable base component for List 2`] = `
 {
-  "css-qk0o61": "&.ListBox-styledScrollbars{scrollbar-color:#BBB #f1f1f1;&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}}&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}",
+  "css-1nwu5vb": "&.ListBox-styledScrollbars{scrollbar-color:#BBB #f1f1f1;&::-webkit-scrollbar{width:10px;height:10px;}&::-webkit-scrollbar-track{background-color:#f1f1f1;}&::-webkit-scrollbar-thumb{background-color:#BBB;border-radius:1rem;}&::-webkit-scrollbar-thumb:hover{background-color:#555;}}",
 }
 `;
diff --git a/apis/nucleus/src/components/listbox/components/grid-list-components/styled-components.js b/apis/nucleus/src/components/listbox/components/grid-list-components/styled-components.js
@@ -37,48 +37,12 @@ export default function getStyledComponents() {
     shouldForwardProp: (prop) => prop !== 'scrollDisabled',
   })(({ scrollDisabled }) => ({
     [`&.${classes.styledScrollbars}`]: getScrollbarStyling(scrollDisabled),
-    // TODO: Verify these props and make generic together with grid component.
-    '&::-webkit-scrollbar': {
-      width: 10,
-      height: 10,
-    },
-
-    '&::-webkit-scrollbar-track': {
-      backgroundColor: scrollBarBackground,
-    },
-
-    '&::-webkit-scrollbar-thumb': {
-      backgroundColor: scrollBarThumb,
-      borderRadius: '1rem',
-    },
-
-    '&::-webkit-scrollbar-thumb:hover': {
-      backgroundColor: scrollBarThumbHover,
-    },
   }));
 
   const StyledFixedSizeGrid = styled(FixedSizeGrid, {
     shouldForwardProp: (prop) => prop !== 'scrollDisabled',
   })(({ scrollDisabled }) => ({
     [`&.${classes.styledScrollbars}`]: getScrollbarStyling(scrollDisabled),
-
-    '&::-webkit-scrollbar': {
-      width: 10,
-      height: 10,
-    },
-
-    '&::-webkit-scrollbar-track': {
-      backgroundColor: scrollBarBackground,
-    },
-
-    '&::-webkit-scrollbar-thumb': {
-      backgroundColor: scrollBarThumb,
-      borderRadius: '1rem',
-    },
-
-    '&::-webkit-scrollbar-thumb:hover': {
-      backgroundColor: scrollBarThumbHover,
-    },
   }));
 
   return {

diff --git a/test/rendering/listbox/listbox.spec.js-snapshots/grid-one-row-js-linux.png b/test/rendering/listbox/listbox.spec.js-snapshots/grid-one-row-js-linux.png