feat(cdk/table): Emit CSS classes on sticky elements that border non-… - #21179
Conversation
google-cla
Bot
added
the
cla: yes
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
Overall looks good, couple minor comments
| this._borderCellCss = { | ||
| 'top': `${_stickCellCss}-border-elem-top`, | ||
| 'bottom': `${_stickCellCss}-border-elem-bottom`, | ||
| 'left': `${_stickCellCss}-border-elem-left`, |
There was a problem hiding this comment.
Would it be accurate to use "start" and "end" so that the terminology works with RTL?
There was a problem hiding this comment.
This is actually accurate - the rtl right/left swapping is already handled in the sticky column code below.
| * direction and value. | ||
| */ | ||
| _addStickyStyle(element: HTMLElement, dir: StickyDirection, dirValue: number) { | ||
| _addStickyStyle(element: HTMLElement, dir: StickyDirection, dirValue: number, |
There was a problem hiding this comment.
I'm not a huge fan of boolean, "do something extra" flags like this, but I can live with it since it's an internal method. I think ideally we'd have a data structure to pass to this method that had these args.
There was a problem hiding this comment.
From a readability perspective I agree, though from a function that gets called hundreds or thousands of times while setting up a table perspective, I'm not sure it's the right trade-off.
| const endPositions = this._getStickyEndColumnPositions(cellWidths, stickyEndStates); | ||
|
|
||
| const lastStickyStart = stickyStartStates.lastIndexOf(true); | ||
| const firstStickyEnd = stickyEndStates.indexOf(true); |
There was a problem hiding this comment.
Do we need some logic to verify that the first and last sticky indexes are different?
There was a problem hiding this comment.
I think as long as the same column is not specified as sticky and stickyEnd, this will be fine. I think that if both of those were set, getting both of these CSS classes would be the least of the odd behaviors to result.
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
LGTM, there's just a lint error on the commit message (you can fix by squashing)
jelbourn
added
G
…sticky elements This allows adding borders and/or shadows to indicate the break between sticky and fixed rows/columns.
1ede76a to
c12a77e
Compare
Done |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
4 participants
…sticky elements
This allows adding borders and/or shadows to indicate the break between sticky
and fixed rows/columns.