Rework table height to support percentages again #13683
Conversation
| @@ -50,8 +51,7 @@ | |||
| metadata: { dataSource: table }, | |||
| }, | |||
| ] | |||
| $: height = $component.styles?.normal?.height || "408px" | |||
| $: styles = getSanitisedStyles($component.styles) | |||
| $: gridContext?.minHeight?.subscribe($height => (minHeight = $height)) | |||
There was a problem hiding this comment.
The core grid is the source of truth and makes min height available in context, which we can consume from here and honour.
There was a problem hiding this comment.
Does unsubscribe ever need to be called on this?
Would storing minHeight as reactive variable and then assigning it to minHeight work?
$: contextMinHeight = gridContext.minHeight;
$: minHeight = $contextMinHeight
Obviously more verbose, but if the unsubscribe is necessary might be worth it.
Very much NAB anyway.
There was a problem hiding this comment.
I have a feeling svelte manages that automatically if called inside a reactive statement, but even if it doesn't then we don't need to worry since this will only every trigger once (when grid context is set for the first time) so we don't need to worry about excess subscriptions.
Good intuition though, because in other cases that could absolutely be a problem.
There was a problem hiding this comment.
Going to move this into the onMount handler to be more explicit about the fact it'll only run once. Makes sense to be explicit about it.
| Padding + SmallRowHeight + $rowHeight + (showControls ? ControlsHeight : 0) | ||
|
|
||
| // Derive min height and make available in context | ||
| const minHeight = derived(rowHeight, $height => { |
There was a problem hiding this comment.
Small change to make this a derived store rather than a primitive, so it's observable.
| @@ -50,8 +51,7 @@ | |||
| metadata: { dataSource: table }, | |||
| }, | |||
| ] | |||
| $: height = $component.styles?.normal?.height || "408px" | |||
| $: styles = getSanitisedStyles($component.styles) | |||
| $: gridContext?.minHeight?.subscribe($height => (minHeight = $height)) | |||
There was a problem hiding this comment.
Does unsubscribe ever need to be called on this?
Would storing minHeight as reactive variable and then assigning it to minHeight work?
$: contextMinHeight = gridContext.minHeight;
$: minHeight = $contextMinHeight
Obviously more verbose, but if the unsubscribe is necessary might be worth it.
Very much NAB anyway.
…licit about it only running once
…se into rework-grid-block-height
Description
Recent changes to the table component meant that using percentages as a height no longer worked. This was because we were not apply height to the outer DOM node, but instead applying it to the inner grid component itself, breaking relativity to the parent of the table component as the user sees it. Those changes were motivated by a desire to ensure that the min height of the table is respected, ensuring users can't create a table that's too small to be used.
This PR reworks how height is calculated and applied so that percentages work again, but we also still use the correct min-height.
Example of a table with a height of 50%, showing the parent container to highlight that it's working:
Addresses