[Table] Make editable cells fill cells vertically #1673
Conversation
cmslewis
changed the title
|
|
||
| private invokeCallback(callback: (value: string, rowIndex?: number, columnIndex?: number) => void, value: string) { | ||
| // pass through the row and column indices if they were provided as props by the consumer | ||
| // const { columnIndex } = this.props; |
| this.invokeCallback(this.props.onConfirm, value); | ||
| }; | ||
|
|
||
| private invokeCallback(callback: (value: string, rowIndex?: number, columnIndex?: number) => void, value: string) { |
There was a problem hiding this comment.
These callback params look inconsistent with the ones in the props interface above.
In the props interface: value, columnIndex
Here: value, rowIndex, columnIndex
packages/table/src/cell/_cell.scss
Outdated
| top: 0; | ||
| right: 10px; | ||
| bottom: 0; | ||
| left: 10px; |
There was a problem hiding this comment.
Are these offsets not accomplished with padding within the editable text component itself?
Also, probably want to use $pt-grid-size. // CC @llorca
There was a problem hiding this comment.
So the issue here is that if I use padding it messes up the ::before absolutely positioned element that shows up to make the input the right size, and messing with those values messes with the editable headers.
I'll do it the other way, though.
|
Gah, my bad, that file shouldn't be in the PR in the first place, was something else I was working on. Whoops. |
Undo spurious added filePreview: documentation | table |
Better way to style thingsPreview: documentation | table |
gscshoyru
changed the title
| } | ||
|
|
||
| &.bp-table-editable-text { | ||
| &::before { |
There was a problem hiding this comment.
I'd merge this selector onto line above, but whatever 👍 &.bp-table-editable-text::before
| &::before { | ||
| top: -$cell-padding-vertical + $cell-border-width - 1px; | ||
| right: $cell-border-width; | ||
| bottom: -$cell-padding-vertical + 1px; |
There was a problem hiding this comment.
seems like top and bottom and cursor don't change from selector above. maybe they can be removed from this block, since it necessarily matches the selector above as well?
| top: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| left: 0; |
There was a problem hiding this comment.
classic absolute fill. why isnt this a blueprint style or sass mixin yet?
There was a problem hiding this comment.
@include position(absolute, 0)There was a problem hiding this comment.
So far as I can tell, this thing doesn't exist, or at least SASS was unhappy with it
There was a problem hiding this comment.
it's a bourbon mixin. search codebase for other usages. but don't block this PR on it cuz #875. http://bourbon.io
Remove unnecessaryPreview: documentation | table |
| top: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| left: 0; |
|
|
Fixes #1235
Changes proposed in this pull request:
Make some style modifications to the surrounding div of the editable text, with a new class, to make it always horizontally fill the cell. Double-clicking anywhere in the cell will open the input now, and it takes up the whole space.
Reviewers should focus on:
Is position: absolute really the best way of doing this? I'm not happy with it, but I couldn't come up with anything better.