Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions addon/components/hyper-table-v2/column.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export default class HyperTableV2Column extends Component<HyperTableV2ColumnArgs
classes.push('hypertable__column--filtered');
}

if (this.stickyColumnClass) {
classes.push(this.stickyColumnClass);
}

return classes.join(' ');
}

Expand All @@ -81,6 +85,14 @@ export default class HyperTableV2Column extends Component<HyperTableV2ColumnArgs
return this.args.column.definition.orderable && !this.args.column.order?.direction;
}

get stickyColumnClass(): string | undefined {
const position = this.args.column.definition?.position;

if (!position?.sticky) return undefined;

return `hypertable__column--sticky-${position.side ?? 'left'}`;
}

@action
setDropdownHeight(): void {
const dropdown = document.querySelector(`#${this.elementId} .available-filters`) as HTMLElement;
Expand Down
4 changes: 0 additions & 4 deletions addon/components/hyper-table-v2/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,6 @@
@handler={{@handler}}
@column={{column}}
@delegatedFiltering={{@options.delegatedFiltering}}
class={{if
column.definition.position.sticky
(concat "hypertable__column--sticky-" column.definition.position.side)
}}
{{sortable-item
groupName=this.hypertableInstanceID
model=column
Expand Down
11 changes: 8 additions & 3 deletions addon/core/interfaces/column.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,14 @@ export type ColumnDefinition = {
facetable: boolean;
facetable_by: string[] | null;
empty_state_message?: string;
position?: {
sticky: boolean;
};
position?:
| {
sticky: false;
}
| {
sticky: true;
side?: 'left' | 'right';
};
Comment on lines +24 to +30
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the side attribute is optional, what's the difference between what's written and something like

position?: { sticky: boolean; side?: 'left | right' }

Copy link
Copy Markdown
Contributor Author

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:

"Property side does not exist on type:"

position:
     {
       sticky: false;
     }

IMHO the intent is clearer.
At runtime the code behaves the same.
It only improves the DX.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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?

};

export type Filter = {
Expand Down
51 changes: 51 additions & 0 deletions tests/integration/components/hyper-table-v2-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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,
but maybe you could also include a test that checks that passing "left" renders with the sticky-left class?
So you cover default + left side + right side :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.
So now we have:

  • no stickiness
  • sticky + default
  • sticky + right
  • sticky + left

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();
});
});
});
Loading