Extend Grid layout with custom rows and columns options and more (#149)
#183
Conversation
|
Awesome! That' exactly what I needed. |
| @@ -25,7 +25,8 @@ | |||
| "rules": { | |||
| "react/jsx-filename-extension": [1, { "extensions": [".mdx"] }], | |||
| "react/jsx-indent": "off", // Gives false positives in MDX files. | |||
| "semi": "off" // We don't want to clutter our MDX with semicolons. | |||
| "semi": "off", // We don't want to clutter our MDX with semicolons. | |||
| "sort-keys": "off" // This rule only needs to be turned off in a few places (eg. breakpoint lists), but weirdly enough, an inline comment alerts breaking of the `indent` rule. | |||
There was a problem hiding this comment.
I would use sort-keys disabled inline. I do not agree with disabling it globally.
There was a problem hiding this comment.
There was a problem hiding this comment.
If it is for MDX only, we can leave it as it is.
| import { generateResponsiveCustomProperties } from './helpers'; | ||
| import styles from './Grid.scss'; | ||
|
|
||
| export const GridSpan = (props) => { |
There was a problem hiding this comment.
I would prefer the destructuring syntax:
export const GridSpan = ({
children,
columns,
id,
rows,
...other
}) => {I know it is not used elsewhere, but I find it easier to read. In fact I think we should update other files to use it as well (not in this PR obviously).
There was a problem hiding this comment.
In fact I think we should update other files to use it as well (not in this PR obviously).
Sure, there is an issue for that: #113.
There was a problem hiding this comment.
And thank you for pointing it out, I missed it somehow this time.
| @@ -0,0 +1,19 @@ | |||
| export const generateResponsiveCustomProperties = (prop, infix) => { | |||
There was a problem hiding this comment.
I would prefer to have one named export per file where the the filename is the name of the function. So this file should be named helpers/generateResponsiveCustomProperties.js
It makes the implementation, but especially the tests much easier to read.
| return null; | ||
| } | ||
|
|
||
| const customProperties = {}; |
There was a problem hiding this comment.
I would replace it with:
if (typeof prop !== 'object') {
return { [`--rui-local-${infix}-xs`]: prop };
}
return Object.keys(prop).reduce((acc, breakpoint) => ({
...acc,
[`--rui-local-${infix}-${breakpoint}`]: prop[breakpoint],
}), {});The return early pattern makes it easier to follow as I can exit the function, well, early as the logical branches do not reconnect.
| @@ -0,0 +1,19 @@ | |||
| export const generateResponsiveCustomProperties = (prop, infix) => { | |||
| if (typeof prop === 'undefined') { | |||
There was a problem hiding this comment.
maybe we should add a unit test…
| import { generateResponsiveCustomProperties } from '../generateResponsiveCustomProperties'; | ||
|
|
||
| describe('generateResponsiveCustomProperties', () => { | ||
| it('when prop is undefined', () => { |
There was a problem hiding this comment.
It is up to you. But I would use test here, it would make sense then.
Closes #149.