-
Notifications
You must be signed in to change notification settings - Fork 0
Fix sticky column logic and ColumnDefinition type #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cc9f283
f610a59
6663969
3a5afba
b481319
f5cf2b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -641,4 +641,55 @@ module('Integration | Component | hyper-table-v2', function (hooks) { | |
| meta: { total: 12 } | ||
| }); | ||
| } | ||
|
|
||
| module('sticky column class', function () { | ||
| test('Sticky attribute not provided - column renders without sticky class', async function (this: TestContext, assert: Assert) { | ||
| sinon.stub(this.tableManager, 'fetchColumns').callsFake(() => | ||
| Promise.resolve({ | ||
| columns: [buildColumn('foo', { position: undefined }), buildColumn('bar', { position: undefined })] | ||
| }) | ||
| ); | ||
|
|
||
| await render(hbs`<HyperTableV2 @handler={{this.handler}} />`); | ||
|
|
||
| assert.dom('.hypertable__column--sticky-left').doesNotExist(); | ||
| assert.dom('.hypertable__column--sticky-right').doesNotExist(); | ||
| }); | ||
|
|
||
| test('Sticky attribute provided with no side - column renders with sticky-left class', async function (this: TestContext, assert: Assert) { | ||
| sinon.stub(this.tableManager, 'fetchColumns').callsFake(() => | ||
| Promise.resolve({ | ||
| columns: [buildColumn('foo'), buildColumn('bar', { position: { sticky: true } })] | ||
| }) | ||
| ); | ||
|
|
||
| await render(hbs`<HyperTableV2 @handler={{this.handler}} />`); | ||
|
|
||
| assert.dom('.hypertable__column--sticky-left').exists(); | ||
| }); | ||
|
|
||
| test('Sticky attribute provided with right side - column renders with sticky-right class', async function (this: TestContext, assert: Assert) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure that's really necessary since you already have a test for the default behavior,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood correctly: I implemented an extra test for the defined left side sticky column.
|
||
| sinon.stub(this.tableManager, 'fetchColumns').callsFake(() => { | ||
| return Promise.resolve({ | ||
| columns: [buildColumn('foo'), buildColumn('bar', { position: { sticky: true, side: 'right' } })] | ||
| }); | ||
| }); | ||
|
|
||
| await render(hbs`<HyperTableV2 @handler={{this.handler}} />`); | ||
|
|
||
| assert.dom('.hypertable__column--sticky-right').exists(); | ||
| }); | ||
|
|
||
| test('Sticky attribute provided with left side - column renders with sticky-left class', async function (this: TestContext, assert: Assert) { | ||
| sinon.stub(this.tableManager, 'fetchColumns').callsFake(() => { | ||
| return Promise.resolve({ | ||
| columns: [buildColumn('foo'), buildColumn('bar', { position: { sticky: true, side: 'left' } })] | ||
| }); | ||
| }); | ||
|
|
||
| await render(hbs`<HyperTableV2 @handler={{this.handler}} />`); | ||
|
|
||
| assert.dom('.hypertable__column--sticky-left').exists(); | ||
| }); | ||
| }); | ||
| }); | ||
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.
since the
sideattribute is optional, what's the difference between what's written and something likeThere 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.
It's a good question.
The difference is tiny to be honest.
With your implementation, one could write with no problem/warning:
position: { sticky: false; side: 'left' }IMHO, this could (maybe) trip up a dev.
But with my implementation, if you write this, you get a TS error message saying something like:
IMHO the intent is clearer.
At runtime the code behaves the same.
It only improves the DX.
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.
FYI: I very briefly talked about it with Phil who told me this syntax was ok for him
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.
Because there's a world where a dev would write sticky false and also specify a side ?
I disagree - but since you've already merged the PR, whatever I guess.
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.
Do you want me to open another PR to make the change to what you proposed?