Export functions used to build className for Row & Column #91
Conversation
This will allow other components to act as a Row or Column by sharing the prop building validation
|
While testing #90, I discovered another use case that'd be nice to support. Also happy to rework and/or add to the README if you think it's a good idea. |
There was a problem hiding this comment.
That's useful! Because sometimes I don't want a containing <div>. How about adding those exports to index.js? That way they can be imported like this:
import { getColClassNames } from 'react-flexbox-grid';Yes, please add this to README.md as well. 👍
What other use case did you had in mind?
|
Just pushed an update to do so. This is all I've thought of, but I am starting a new project where I'm planning to use |
README.md
Outdated
| Functions for generating Row and Column classNames are exported for use in other components. For example, suppose you have a `MyFormInput` component that should also act as both a `Row` and a `Col`. | ||
|
|
||
| ```js | ||
| { |
There was a problem hiding this comment.
I think this { is accidental?
src/index.js
Outdated
| export Row from './components/Row'; | ||
| export Col from './components/Col'; | ||
| export * from './components/Row'; | ||
| export * from './components/Col'; |
There was a problem hiding this comment.
I would rather if these exports were explicit, just so we don't accidentally expose some internal API in the future. Combined with leaving Col and Row as default exports, I think this should be:
export Row, { getRowClassNames } from './components/Row';
export Col, { getColClassNames } from './components/Col';Not sure if this is valid syntax, though. 😅
There was a problem hiding this comment.
yes, that's correct - I wrote a spec to test that everything is exported as it should be. I was on the fence about doing as you suggest vs the glob. I went with the glob because it'd make future changes easier to make, but explicit is also a good concern. Made an update
test/components/Col.spec.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| import expect from 'expect'; | |||
| import React from 'react'; | |||
| import TestUtils from 'react-addons-test-utils'; | |||
| import Col from '../../src/components/Col'; | |||
| import { Col, getColClassNames } from '../../src/components/Col'; | |||
There was a problem hiding this comment.
If we're keeping Col as default, as explained above:
import Col, { getColClassNames } from '../../src/components/Col';
test/components/Row.spec.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| import expect from 'expect'; | |||
| import React from 'react'; | |||
| import TestUtils from 'react-addons-test-utils'; | |||
| import Row from '../../src/components/Row'; | |||
| import { Row, getRowClassNames } from '../../src/components/Row'; | |||
src/components/Col.js
Outdated
| @@ -60,8 +60,8 @@ function getClassNames(props) { | |||
| .concat(extraClasses); | |||
| } | |||
|
|
|||
| export default function Col(props) { | |||
| const classNames = getClassNames(props); | |||
| export function Col(props) { | |||
There was a problem hiding this comment.
I see no reason for converting this default export to a named one, I see this only as a risk of breaking something. I described below how leaving this as a default export would reflect on the rest of the changes.
src/components/Row.js
Outdated
| @@ -38,8 +38,8 @@ function getClassNames(props) { | |||
| return modificators; | |||
| } | |||
|
|
|||
| export default function Row(props) { | |||
| return React.createElement(props.tagName || 'div', createProps(propTypes, props, getClassNames(props))); | |||
| export function Row(props) { | |||
|
No problem at all - this is your baby; I'm just glad you're willing to accept updates so we can all use it as we'd like. Just made another round for you to look at. |
|
Looks good, thanks! 👍 |
| export default function MyFormInput(props) { | ||
| return ( | ||
| <form className={getRowclassNames(props)}> | ||
| <input className={getColClassNames(props)} /> |
There was a problem hiding this comment.
I don't think this example will work correctly, because getRowClasssNames and getColClassNames produce an array, but className requires a string. When I was playing with this, I got something like this:
<form class=",row,around-xs">I think it relies on createProps to concatenate it to a valid class name.
There was a problem hiding this comment.
Hm, yes good point. I'm actually feeding it into the classnames lib which seems smart enough to be do the right thing. I'll play with it a bit to find the best approach, then make a PR.
This will allow other components to act as a Row or Column by sharing the same prop building logic.
For instance I have a
<FormField />component (who doesn't?). I could wrap it withCol, but it would be nice if it would accept all theColprops and set the appropriate classNames on itself.