Add ability to add optional css class for checkbox cell #2951
Conversation
|
|
| /** | ||
| * Boolean value to indicate if the cells should be vertically centered rather than top-aligned | ||
| */ | ||
| verticallyCenterCells?: boolean; |
There was a problem hiding this comment.
@cschleiden What do you think? This is what we've resolved:
- The default of vertically align top makes the most sense
- In some cases, we need to center the content
So the #2 is what @MaxLustig is dealing with and right now it's tricky to do so.
Maybe this should just be done through column definitions. I think defaulting every cell to center might not be desired in every column.
I think column takes a className that gets injected on each cell, could that be a solution here?
There was a problem hiding this comment.
I'm really, really against this property. We have to be more modular, not less. Can't you do it with an onRender callback in the column?
There was a problem hiding this comment.
I tested out using className on the column definition, which works perfectly well. That'll work for my scenario at least. However, there was a change last week that vertically centers the checkmark in details row, which I had fixed as part of this change. Should I just get rid of all the centering code and keep that change or open up a new PR?
There was a problem hiding this comment.
Actually, I found one issue with using className in column definition, in that the checkbox does not take a className (and therefore cannot be centered). I've updated the PR to enable providing a css class for the checkbox's cell.
MaxLustig
changed the title
| /** | ||
| * Optional class name to add to the cell of a checkbox | ||
| */ | ||
| checkboxClassName?: string; |
There was a problem hiding this comment.
We already have on check... props, should we go all the way and take ICheckProps?
There was a problem hiding this comment.
Okay, synced offline. the DetailsCheck and CheckProps have their own issues so it might be fine.
Only complaint, while the comment explains correctly that the class is added to the cell of the checkbox, the name doesn't match that. Can we find a better one?
| /** | ||
| * Optional class name to add to the cell of a checkbox | ||
| */ | ||
| checkboxClassName?: string; |
There was a problem hiding this comment.
Okay, synced offline. the DetailsCheck and CheckProps have their own issues so it might be fine.
Only complaint, while the comment explains correctly that the class is added to the cell of the checkbox, the name doesn't match that. Can we find a better one?
MaxLustig
force-pushed
the
maxlus/verticallyCenterCells
branch
from
8241e64 to
bd2218e
Compare
* Added property to IDetailsListProps to support vertically centering cells * Fixed vertical centering issue with checkbox * Added change file * Removed verticallyCenterCells * Added ability to add css class to checkbox cell * Fixed issue with DetailsRowProps declaration * Changed change file * Changed name to checkboxCellClassName * Update maxlus-verticallyCenterCells_2017-09-26-21-11.json
* Added property to IDetailsListProps to support vertically centering cells * Fixed vertical centering issue with checkbox * Added change file * Removed verticallyCenterCells * Added ability to add css class to checkbox cell * Fixed issue with DetailsRowProps declaration * Changed change file * Changed name to checkboxCellClassName * Update maxlus-verticallyCenterCells_2017-09-26-21-11.json
4 participants
Pull request checklist
$ npm run changeDescription of changes
By default, cells in a details list are top-aligned. They can be made vertically centered using className in the IColumn definition for each column, but the checkbox cell does not take a className as a property. This adds the ability to provide a className to be applied to the checkbox's containing cell.