Skip to content

Svelte5 Fixes #11

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Svelte5 Fixes #11

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fea81e5
Removed some warninings
Apr 30, 2025
335ca52
Fixed ARIA rules
Apr 30, 2025
fcc3cc6
Fixed reactivity for sortOrder
Apr 30, 2025
6a371ec
New approach
May 6, 2025
3f0fbdb
sticky header for VirtualTable component
May 14, 2025
13b52d8
All is working. Need to change the row height from outside.
May 16, 2025
8458aa9
Everything is working with 40px. Now checking the option to work with…
May 16, 2025
849d507
now it is better
May 16, 2025
530d412
update comments
May 16, 2025
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
74 changes: 51 additions & 23 deletions src/lib/VirtualTable.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
let headHeight = $state(0);
let footHeight = $state(0);
let heightMap = $state([]);
let mounted = $state();
let rows = $state();
let mounted = $state(false);
let rows = $state(null);
let theadElement = $state();
let top = $state(0);
let viewport = $state();
Expand All @@ -41,6 +41,7 @@
console.log('Deriving sorted items');
return sorted([...items], sortOrder);
});
let visible = $derived.by(() => {
console.log('Deriving visible items', [start, end]);
return sortedItems.slice(start, end).map((data, i) => {
Expand All @@ -50,9 +51,15 @@
// whenever `items` changes, invalidate the current heightmap
$effect(() => {
if (items && mounted) {
console.log('Refreshing height map for ' + items.length + ' items');
refreshHeightMap(items, viewportHeight, itemHeight);
console.debug('Effect triggered: mounted =', mounted, 'items.length =', items.length);
if (mounted) {
if (items.length > 0) {
console.log('Refreshing height map for ' + items.length + ' items');
refreshHeightMap(items, viewportHeight, itemHeight);
} else {
console.debug('No items to refresh height map');
}
}
});
Expand Down Expand Up @@ -174,6 +181,13 @@
}
export async function scrollToIndex(index, opts) {
if (viewport) {
console.log('viewport is ready.');
} else {
console.warn('viewport is null or undefined.');
return;
}
const { scrollTop } = viewport;
const itemsDelta = index - start;
const _itemHeight = itemHeight || averageHeight;
Expand All @@ -188,9 +202,9 @@
}
// MARK: table sort stuff
let sortOrder = [[]];
let sortOrder = $state([[]]);
const sorted = function (arr, sortOrder) {
const sorted = (arr, sortOrder) => {
arr.sort((a, b) => {
for (let [fieldName, r] of sortOrder) {
const reverse = r === 0 ? 1 : -1;
Expand Down Expand Up @@ -252,24 +266,39 @@
onMount(() => {
// triggger initial refresh for virtual
rows = contents.children;
[...rows].forEach((trElement) => {
const firstTh = trElement.querySelector('th:first-child');
if (firstTh) {
debugger;
console.log('firstTh exists');
firstTh.setAttribute('role', 'rowheader'); // Row header for row labels
} else {
console.log('firstTh is null');
}
});
mounted = true;
console.debug('Before in onMount');
refreshHeightMap(items, viewportHeight, itemHeight);
console.debug('After in onMount');
// prepare sorting
const th = theadElement.getElementsByTagName('th');
for (let i = 0; i < th.length; i++) {
if (th[i].dataset.sort) {
th[i].className = CLASSNAME_SORTABLE;
th[i].onclick = (event) => updateSortOrder(th[i], event.shiftKey);
const thElements = theadElement.getElementsByTagName('th');
[...thElements].forEach((th) => {
th.setAttribute('role', 'columnheader');
if (th.dataset.sort) {
th.className = CLASSNAME_SORTABLE;
th.onclick = (event) => updateSortOrder(th, event.shiftKey);
}
if (th[i].dataset.sortInitial === 'descending') {
th[i].className = CLASSNAME_SORTABLE + ' ' + CLASSNAME_DESC;
sortOrder = [...sortOrder, [th[i].dataset.sort, 1]];
} else if (th[i].dataset.sortInitial != undefined) {
th[i].className = CLASSNAME_SORTABLE + ' ' + CLASSNAME_ASC;
sortOrder = [...sortOrder, [th[i].dataset.sort, 0]];
if (th.dataset.sortInitial === 'descending') {
th.className = CLASSNAME_SORTABLE + ' ' + CLASSNAME_DESC;
sortOrder = [...sortOrder, [th.dataset.sort, 1]];
} else if (th.dataset.sortInitial !== undefined) {
th.className = CLASSNAME_SORTABLE + ' ' + CLASSNAME_ASC;
sortOrder = [...sortOrder, [th.dataset.sort, 0]];
}
}
});
});
</script>

Expand All @@ -281,18 +310,17 @@
bind:this={viewport}
bind:offsetHeight={viewportHeight}
onscroll={handleScroll}
role="table"
style="height: {height}; --bw-svt-p-top: {top}px; --bw-svt-p-bottom: {bottom}px; --bw-svt-head-height: {headHeight}px; --bw-svt-foot-height: {footHeight}px; --bw-svt-avg-row-height: {averageHeight}px"
>
<thead class="thead" bind:this={theadElement} bind:offsetHeight={headHeight} role="rowheader">
<thead class="thead" bind:this={theadElement} bind:offsetHeight={headHeight}>
{@render thead?.()}
</thead>
<tbody bind:this={contents} class="tbody" role="rowgroup">
<tbody bind:this={contents} class="tbody">
{#each visible as item, i}
{@render trow?.(item.data, item.index, i)}
{/each}
</tbody>
<tfoot class="tfoot" bind:offsetHeight={footHeight} role="rowgroup">
<tfoot class="tfoot" bind:offsetHeight={footHeight}>
Copy link
Contributor

@GenieTim GenieTim Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The reason these role attributes are required (particularly the role="table") (I know, Svelte gives warnings) is that we set display: block on the <table>, which destroys the automatic a11y semantics that the Svelte compiler believes are sufficient, i.e., they are not sufficient anymore and we need to override what CSS suggests again by using these role attributes. So, please don't remove them and accept the warnings, or convince me that modern Browsers don't show this behaviour anymore.

Copy link
Contributor

@GenieTim GenieTim Apr 30, 2025

Choose a reason for hiding this comment

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

I notice that you set some of the attributes from the JavaScript code, but different ones / not the same. What am I misunderstanding?

Copy link
Author

@RandallFlagg RandallFlagg May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I notice that you set some of the attributes from the JavaScript code, but different ones / not the same. What am I misunderstanding?

I assumed that the purpose of the roles on the th was to be ARIA compliant so I put it on the th in both, thead and tbody.

So, please don't remove them and accept the warnings, or convince me that modern Browsers don't show this behaviour anymore.

Regarding the table role, why do you set the table to a display different than table?

Copy link
Contributor

@GenieTim GenieTim May 1, 2025

Choose a reason for hiding this comment

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

Regarding the table role, why do you set the table as display different than table?

See https://github.com/BernhardWebstudio/svelte-virtual-table?tab=readme-ov-file#development-notes – I just did not manage to get any of the other methods I could think of working reliably. But honestly, it's been a while, maybe I would come up with different solutions today. If you manage, please, I will gladly accept the PR or hand the library over to you.

Copy link
Author

@RandallFlagg RandallFlagg May 1, 2025

Choose a reason for hiding this comment

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

As these are not block-type elements, the original intention to use padding as a means to indicate the table's "scrollability" of the inner table is not possible.

I want to make sure I understand what you mean. You want the table to show a scrollbar on it? That is the issue here?

Also, please exlplain what is the purpose of this attribute: requireBorderCollapse.

Copy link
Contributor

@GenieTim GenieTim May 2, 2025

Choose a reason for hiding this comment

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

In this example, thanks, I do not encounter the column jumping issue anymore. Rows still jump, it feels a bit like a ratchet, rather than smooth scrolling. As you say, you fix them as you go, so I will gladly wait for your PR with a smoothly working example.

You could have put SPAN instead or just give it what evername you want and put roles on it. That will be the same.

Oh, yes, absolutely, that's why the role attributes are needed in the current implementation. I am simply using the "original" tags in order to have logical semantics for users, because why change.

Copy link
Author

@RandallFlagg RandallFlagg May 2, 2025

Choose a reason for hiding this comment

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

As you say, you fix them as you go, so I will gladly wait for your PR with a smoothly working example.

I need you to provide me the scenarios so I can solve them. Try to be as accurate as possible. You are now my QA for that matter.

Oh, yes, absolutely, that's why the role attributes are needed in the current implementation. I am simply using the "original" tags in order to have logical semantics for users, because why change.

Because you might forget a few(e.g. rowheader) and people expect a certain behaviour which doesn't happen because it is a block and not a table. I am sure there are a few more reasons but you get the point.

Copy link
Contributor

@GenieTim GenieTim May 2, 2025

Choose a reason for hiding this comment

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

I need you to provide me the scenarios so I can solve them.

What scenarios? You mean, just test your table for you? Make sure to have examples where

  1. Not all rows have the same height (e.g. rows with images, multi-line text; images in particular are "dangerous" in the sense that they will only load once they scroll into view),
  2. You have fewer rows than needed,
  3. You have and don't have header and/or footer

Because you might forget a few(e.g. rowheader) and people expect a certain behaviour which doesn't happen because it is a block and not a table.

You don't need to convince me that it's better not to use display: block on the table, I know that and fully agree, which is why I will accept any alternative implementation if it manages to scroll as good as the current implementation.

Copy link
Author

@RandallFlagg RandallFlagg May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Not all rows have the same height (e.g. rows with images, multi-line text; images in particular are "dangerous" in the sense that they will only load once they scroll into view),

https://output.jsbin.com/zejemuy/

  1. You have fewer rows than needed,

Please explain what is the requirment

  1. You have and don't have header and/or footer

I tried it with and without header and footer and it seems to work good

Copy link
Contributor

@GenieTim GenieTim May 2, 2025

Choose a reason for hiding this comment

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

https://output.jsbin.com/zejemuy/

This example does not work for me at all; visible rows = 0, a long scrolling white page otherwise.

Please explain what is the requirment

You have fewer rows than needed: sorry, that's indeed a suboptimal formulation, let's try again: you have data for fewer rows, than the potential height of the table, i.e., there is no need for anything virtual, nothing being not-rendered. Make sure that in this case the rows are not stretched vertically.

{@render tfoot?.()}
</tfoot>
</table>
Expand Down
1 change: 1 addition & 0 deletions src/routes/+page.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const ssr = false;
23 changes: 13 additions & 10 deletions src/routes/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,35 @@
<script>
export const ssr = false;
import { onMount } from 'svelte';
import VirtualTable from '../lib/index';
// import VirtualTable from 'svelte-virtual-table'

let items = $state([]);
let searchTerm = $state('');
let start = $state(0);
let start2 = $state(0);
let end = $state(10);
let end2 = $state(10);

let dataPromise = $state(null);
async function getData() {
let dataItems = [];
for (let page = 1; page < 5; page++) {
let res = await fetch(`https://node-hnapi.herokuapp.com/news?page=${page}`);
let data = await res.json();
dataItems = dataItems.concat(data);
//dataItems = [...dataItems, ...data];
}
items = dataItems;
return items;
}

const dataPromise = getData();

let searchTerm = $state('');
onMount(() => {
dataPromise = getData(); // This ensures it updates reactively and correctly
});

let filteredList = $derived(
items.filter((item) => item.title.toUpperCase().indexOf(searchTerm.toUpperCase()) !== -1)
items.filter((item) => item.title.toUpperCase().includes(searchTerm.toUpperCase()))
);

let start = $state(0),
start2 = $state(0);
let end = $state(10),
end2 = $state(10);
</script>

<h1>Virtual Table Test</h1>
Expand Down