Svelte5 Fixes #11
Svelte5 Fixes #11
Conversation
|
Thank you very much for your contribution! |
src/lib/VirtualTable.svelte
Outdated
| {#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}> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I notice that you set some of the attributes from the JavaScript code, but different ones / not the same. What am I misunderstanding?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- 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),
- You have fewer rows than needed,
- 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.
There was a problem hiding this comment.
- 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/
- You have fewer rows than needed,
Please explain what is the requirment
- 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
There was a problem hiding this comment.
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.
There are a few fixes in this PR: