-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
Just started reviewing :) |
packages/clay-grid/package.json
Outdated
"babel-plugin-transform-node-env-inline": "^0.1.1", | ||
"babel-preset-env": "^1.6.0", | ||
"browserslist-config-clay-components": "^1.0.0-alpha.2", | ||
"clay": "liferay/clay#develop", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not it be the current version of Clay?
packages/clay-grid/src/ClayGrid.js
Outdated
* Flag to indicate if the list group items are selectable. | ||
* @instance | ||
* @memberof ClayGrid | ||
* @type {?bool|undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of just defining ?bool
, since when we do not pass anything we put default is false
?
packages/clay-grid/src/ClayGrid.js
Outdated
/** | ||
* Metal ClayGrid component. | ||
*/ | ||
class ClayGrid extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @carloslancha, I think this name is much more generic than leaving it just for the listing of cards, we can give another impression by the name of the component. Maybe rename it to ClayCardGrid
, ClayCardList
or ClayCardGroup
. What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 ClayCardGrid
my namespace on Clay was based on this item https://i5.walmartimages.com/asr/1fc1aeeb-c02f-4ea7-9ab7-7a0e757bd171_1.a8424a26d75bcc472dda2395f8e5fc77.jpeg
Merged, thanks! |
This is done over clay's develop branch.