Fix sticky column logic and ColumnDefinition type#267
Conversation
Co-authored-by: Copilot <copilot@github.com>
edouardmisset
requested review from
OwenCoogan and
olxmpe
and removed request for
a team
edouardmisset
changed the title
Co-authored-by: Copilot <copilot@github.com>
| | { | ||
| sticky: false; | ||
| } | ||
| | { | ||
| sticky: true; | ||
| side?: 'left' | 'right'; | ||
| }; |
There was a problem hiding this comment.
since the side attribute is optional, what's the difference between what's written and something like
position?: { sticky: boolean; side?: 'left | right' }
There was a problem hiding this comment.
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
sidedoes not exist on type:"position: { sticky: false; }
IMHO the intent is clearer.
At runtime the code behaves the same.
It only improves the DX.
There was a problem hiding this comment.
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.
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.
Do you want me to open another PR to make the change to what you proposed?
| } | ||
|
|
||
| module('sticky column class', function () { | ||
| test('no sticky - column renders without sticky class', async function (this: TestContext, assert: Assert) { |
There was a problem hiding this comment.
Nit: label wording is bit weird - are we talking about the attribute or the feature ?
maybe Sticky attribute not provided - column renders without sticky class is more adapted
There was a problem hiding this comment.
labels for the two tests below can be updated too
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
| 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) { |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
upfluence-bot
added
reviewed/needs-review
reviewed/needs-work
and removed
reviewed/needs-work
reviewed/needs-review
labels
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/ready-to-merge
reviewed/needs-work
labels
8 participants
What does this PR do?
Fixes sticky column class computation in HyperTable v2 and aligns
ColumnDefinition.positiontyping with actual sticky behavior/side usage.Related to : #DRA-5002
What are the observable changes?
Sticky column class is now computed in component logic preventing invalid class generation when position data is missing.
Sticky side handling now supports both left and right (⚠️ I took the assumption that the default sticky side is the left side (which seems to be the default behavior in the app) so as to not break the upfluence-web.
hypertable__column--sticky-left/hypertable__column--sticky-right), defaulting to left when sticky is enabled without an explicit side.ColumnDefinition.positionnow uses a discriminated union:{ sticky: false }{ sticky: true; side?: 'left' | 'right' }This improves type safety and matches runtime behavior.
🧑💻 Developer Heads Up
Good PR checklist
Additional Notes