Limit Grid gap props to accept only available spacing values (#402)

## Migration Path For `columnGap` and `rowGap`, reduce all values (responsive or not) in the format `var(--rui-spacing-X)` to corresponding numbers (just `X`), e.g.: ```jsx // Before <Grid columnGap="var(--rui-spacing-3)"> // After <Grid columnGap={3}> ``` (Please mind the correct value type.)
react-ui-org · Aug 20, 2022 · 42529d4 · 42529d4
1 parent 1b0d005
commit 42529d4
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 48 deletions.
diff --git a/src/docs/contribute/api.mdx b/src/docs/contribute/api.mdx
@@ -27,3 +27,9 @@ instance?
 - If not, put it into the theme. Developers can change it
   [in their theme](/customize/theming/overview) and it will be the same for all
   component instances.
+
+## Measures
+
+Always use [spacing values](/foundation/spacing) for all kinds of measures like
+offsets, gaps, or spacings. This helps keep the design consistent across
+components.
diff --git a/src/lib/components/Grid/Grid.jsx b/src/lib/components/Grid/Grid.jsx
@@ -6,6 +6,8 @@ import { transferProps } from '../_helpers/transferProps';
 import { generateResponsiveCustomProperties } from './_helpers/generateResponsiveCustomProperties';
 import styles from './Grid.scss';
 
+const SPACING_VALUES = [0, 1, 2, 3, 4, 5, 6, 7];
+
 export const Grid = ({
   alignContent,
   alignItems,
@@ -32,9 +34,9 @@ export const Grid = ({
       className={styles.root}
       style={{
         ...generateResponsiveCustomProperties(columns, 'columns'),
-        ...generateResponsiveCustomProperties(columnGap, 'column-gap'),
+        ...generateResponsiveCustomProperties(columnGap, 'column-gap', 'spacing'),
         ...generateResponsiveCustomProperties(rows, 'rows'),
-        ...generateResponsiveCustomProperties(rowGap, 'row-gap'),
+        ...generateResponsiveCustomProperties(rowGap, 'row-gap', 'spacing'),
         ...generateResponsiveCustomProperties(autoFlow, 'auto-flow'),
         ...generateResponsiveCustomProperties(alignContent, 'align-content'),
         ...generateResponsiveCustomProperties(alignItems, 'align-items'),
@@ -55,12 +57,12 @@ Grid.defaultProps = {
   alignItems: undefined,
   autoFlow: undefined,
   children: null,
-  columnGap: 'var(--rui-spacing-4)',
+  columnGap: 4,
   columns: '1fr',
   id: undefined,
   justifyContent: undefined,
   justifyItems: undefined,
-  rowGap: 'var(--rui-spacing-4)',
+  rowGap: 4,
   rows: 'auto',
   tag: 'div',
 };
@@ -119,19 +121,19 @@ Grid.propTypes = {
    */
   children: PropTypes.node,
   /**
-   * Gap between columns. Accepts any valid value of `grid-column-gap` CSS property.
-   * See [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap) for more.
+   * Gap between columns. Accepts any of [spacing values](/foundation/spacing-values) as number.
+   * See [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap) for more about `column-gap`.
    */
   columnGap: PropTypes.oneOfType([
-    PropTypes.string,
+    PropTypes.oneOf(SPACING_VALUES),
     PropTypes.shape({
-      xs: PropTypes.string,
-      sm: PropTypes.string,
-      md: PropTypes.string,
-      lg: PropTypes.string,
-      xl: PropTypes.string,
-      x2l: PropTypes.string,
-      x3l: PropTypes.string,
+      xs: PropTypes.oneOf(SPACING_VALUES),
+      sm: PropTypes.oneOf(SPACING_VALUES),
+      md: PropTypes.oneOf(SPACING_VALUES),
+      lg: PropTypes.oneOf(SPACING_VALUES),
+      xl: PropTypes.oneOf(SPACING_VALUES),
+      x2l: PropTypes.oneOf(SPACING_VALUES),
+      x3l: PropTypes.oneOf(SPACING_VALUES),
     }),
   ]),
   /**
@@ -187,19 +189,19 @@ Grid.propTypes = {
     }),
   ]),
   /**
-   * Gap between rows. Accepts any valid value of `grid-row-gap` CSS property.
-   * See [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/row-gap) for more.
+   * Gap between rows. Accepts any of [spacing values](/foundation/spacing-values) as number.
+   * See [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/row-gap) for more about `row-gap`.
    */
   rowGap: PropTypes.oneOfType([
-    PropTypes.string,
+    PropTypes.oneOf(SPACING_VALUES),
     PropTypes.shape({
-      xs: PropTypes.string,
-      sm: PropTypes.string,
-      md: PropTypes.string,
-      lg: PropTypes.string,
-      xl: PropTypes.string,
-      x2l: PropTypes.string,
-      x3l: PropTypes.string,
+      xs: PropTypes.oneOf(SPACING_VALUES),
+      sm: PropTypes.oneOf(SPACING_VALUES),
+      md: PropTypes.oneOf(SPACING_VALUES),
+      lg: PropTypes.oneOf(SPACING_VALUES),
+      xl: PropTypes.oneOf(SPACING_VALUES),
+      x2l: PropTypes.oneOf(SPACING_VALUES),
+      x3l: PropTypes.oneOf(SPACING_VALUES),
     }),
   ]),
   /**

diff --git a/src/lib/components/Grid/README.mdx b/src/lib/components/Grid/README.mdx
@@ -42,10 +42,9 @@ See [API](#api) for all available options.
 
 - This component implements the native [CSS grid layout][grid-layout], a
   powerful tool for two-dimensional layouts. You may use any value accepted by
-  [grid-template-columns], [grid-template-rows], [grid-column-gap],
-  [grid-row-gap], [grid-auto-flow], [align-content], [align-items],
-  [justify-content], [justify-items], and CSS properties in corresponding API
-  options of the component.
+  [grid-template-columns], [grid-template-rows], [grid-auto-flow],
+  [align-content], [align-items], [justify-content], [justify-items], and CSS
+  properties in corresponding API options of the component.
 
 - To align your items in the grid, **simply wrap** them with the Grid
   component. Unlike other grid frameworks and UI libraries, **no additional
@@ -126,10 +125,11 @@ Use `columns` and `rows` to specify a more complicated grid layout.
 
 ## Gaps
 
-Both column and row gaps can be customized.
+Both column and row gaps can be customized. The value must correspond to any of
+[available spacings](/foundation/spacing).
 
 <Playground>
-  <Grid columns="repeat(3, 1fr)" columnGap="0.75rem" rowGap="2rem">
+  <Grid columns="repeat(3, 1fr)" columnGap={2} rowGap={6}>
     <Placeholder bordered>Grid item</Placeholder>
     <Placeholder bordered>Grid item</Placeholder>
     <Placeholder bordered>Grid item</Placeholder>
@@ -197,17 +197,17 @@ breakpoints are used until they're overridden by a bigger breakpoint.
       md: '1fr 2fr',
     }}
     columnGap={{
-      xs: 'var(--rui-spacing-4)',
-      md: 'var(--rui-spacing-2)',
-      lg: 'var(--rui-spacing-4)',
+      xs: 4,
+      md: 2,
+      lg: 4,
     }}
     rows={{
       xs: 'auto auto 200px 200px',
       md: 'auto 200px auto',
     }}
     rowGap={{
-      xs: 'var(--rui-spacing-3)',
-      md: 'var(--rui-spacing-4)',
+      xs: 3,
+      md: 4,
     }}
   >
     <Placeholder bordered>Grid item</Placeholder>
@@ -276,8 +276,6 @@ Wrapper for content that should span multiple rows or columns.
 [grid-layout]: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout
 [grid-template-columns]: https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template-columns
 [grid-template-rows]: https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template-rows
-[grid-column-gap]: https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap
-[grid-row-gap]: https://developer.mozilla.org/en-US/docs/Web/CSS/row-gap
 [grid-auto-flow]: https://developer.mozilla.org/en-US/docs/Web/CSS/grid-auto-flow
 [align-content]: https://developer.mozilla.org/en-US/docs/Web/CSS/align-content
 [align-items]: https://developer.mozilla.org/en-US/docs/Web/CSS/align-items

diff --git a/src/lib/components/Grid/__tests__/Grid.test.jsx b/src/lib/components/Grid/__tests__/Grid.test.jsx
@@ -18,6 +18,16 @@ const responsiveBreakpoints = {
   x2l: 'placeholder-x2l',
   x3l: 'placeholder-x3l',
 };
+
+const responsiveSpacingBreakpoints = {
+  xs: 1,
+  sm: 2,
+  md: 3,
+  lg: 4,
+  xl: 5,
+  x2l: 6,
+  x3l: 7,
+};
 /* eslint-enable sort-keys */
 
 const responsiveStyles = (infix) => ({
@@ -30,6 +40,16 @@ const responsiveStyles = (infix) => ({
   [`--rui-local-${infix}-x3l`]: 'placeholder-x3l',
 });
 
+const responsiveSpacingStyles = (infix) => ({
+  [`--rui-local-${infix}-xs`]: 'var(--rui-spacing-1)',
+  [`--rui-local-${infix}-sm`]: 'var(--rui-spacing-2)',
+  [`--rui-local-${infix}-md`]: 'var(--rui-spacing-3)',
+  [`--rui-local-${infix}-lg`]: 'var(--rui-spacing-4)',
+  [`--rui-local-${infix}-xl`]: 'var(--rui-spacing-5)',
+  [`--rui-local-${infix}-x2l`]: 'var(--rui-spacing-6)',
+  [`--rui-local-${infix}-x3l`]: 'var(--rui-spacing-7)',
+});
+
 const defaultProps = {
   children: <div>content</div>,
 };
@@ -66,12 +86,12 @@ describe('rendering', () => {
       (rootElement) => expect(within(rootElement).getByText('content text')),
     ],
     [
-      { columnGap: responsiveBreakpoints },
-      (rootElement) => expect(rootElement).toHaveStyle(responsiveStyles('column-gap')),
+      { columnGap: responsiveSpacingBreakpoints },
+      (rootElement) => expect(rootElement).toHaveStyle(responsiveSpacingStyles('column-gap')),
     ],
     [
-      { columnGap: 'placeholder' },
-      (rootElement) => expect(rootElement).toHaveStyle({ '--rui-local-column-gap-xs': 'placeholder' }),
+      { columnGap: 0 },
+      (rootElement) => expect(rootElement).toHaveStyle({ '--rui-local-column-gap-xs': 'var(--rui-spacing-0)' }),
     ],
     [
       { columns: responsiveBreakpoints },
@@ -99,12 +119,12 @@ describe('rendering', () => {
       (rootElement) => expect(rootElement).toHaveStyle({ '--rui-local-justify-items-xs': 'placeholder' }),
     ],
     [
-      { rowGap: responsiveBreakpoints },
-      (rootElement) => expect(rootElement).toHaveStyle(responsiveStyles('row-gap')),
+      { rowGap: responsiveSpacingBreakpoints },
+      (rootElement) => expect(rootElement).toHaveStyle(responsiveSpacingStyles('row-gap')),
     ],
     [
-      { rowGap: 'placeholder' },
-      (rootElement) => expect(rootElement).toHaveStyle({ '--rui-local-row-gap-xs': 'placeholder' }),
+      { rowGap: 0 },
+      (rootElement) => expect(rootElement).toHaveStyle({ '--rui-local-row-gap-xs': 'var(--rui-spacing-0)' }),
     ],
     [
       { rows: responsiveBreakpoints },

diff --git a/src/lib/components/Grid/_helpers/__tests__/generateResponsiveCustomProperties.test.js b/src/lib/components/Grid/_helpers/__tests__/generateResponsiveCustomProperties.test.js
@@ -7,6 +7,12 @@ describe('generateResponsiveCustomProperties', () => {
     ).toEqual(null);
   });
 
+  test('with prop that is a spacing value', () => {
+    expect(
+      generateResponsiveCustomProperties(3, 'columns', 'spacing'),
+    ).toEqual({ '--rui-local-columns-xs': 'var(--rui-spacing-3)' });
+  });
+
   test('with prop that is not an object', () => {
     expect(
       generateResponsiveCustomProperties('1fr 1fr', 'columns'),

diff --git a/src/lib/components/Grid/_helpers/generateResponsiveCustomProperties.js b/src/lib/components/Grid/_helpers/generateResponsiveCustomProperties.js
@@ -1,14 +1,22 @@
-export const generateResponsiveCustomProperties = (prop, infix) => {
+const prepareValueByType = (value, type) => {
+  if (type === 'spacing') {
+    return `var(--rui-spacing-${value})`;
+  }
+
+  return value;
+};
+
+export const generateResponsiveCustomProperties = (prop, infix, type = null) => {
   if (typeof prop === 'undefined') {
     return null;
   }
 
   if (typeof prop !== 'object') {
-    return { [`--rui-local-${infix}-xs`]: prop };
+    return { [`--rui-local-${infix}-xs`]: prepareValueByType(prop, type) };
   }
 
   return Object.keys(prop).reduce((acc, breakpoint) => ({
     ...acc,
-    [`--rui-local-${infix}-${breakpoint}`]: prop[breakpoint],
+    [`--rui-local-${infix}-${breakpoint}`]: prepareValueByType(prop[breakpoint], type),
   }), {});
 };