Improves UI issues when rendering a Kino.DataTable#497
Conversation
jonatanklosko
left a comment
jonatanklosko
left a comment
There was a problem hiding this comment.
Looks good to me, I have a few comments, but I experimented with it and applied the comments already. Let me know if it works as expected for you :)
assets/packs/data_table/main.js
Outdated
| fontPreloader.innerHTML = | ||
| "<span style=\"font-family:'JetBrains Mono'\">Font preload</span>" + | ||
| "<span style=\"font-family:'Inter'\">Font preload</span>"; |
There was a problem hiding this comment.
We need to have a sample text for every font width, otherwise only the width 400 is loaded for each.
| if (document.fonts && document.fonts.ready) { | ||
| await Promise.race([ | ||
| document.fonts.ready, | ||
| new Promise((resolve) => setTimeout(resolve, FONT_LOAD_TIMEOUT)), | ||
| ]); | ||
| } else { | ||
| await new Promise((resolve) => setTimeout(resolve, FONT_LOAD_TIMEOUT)); | ||
| } |
There was a problem hiding this comment.
We shouldn't do the timeout.
When document.fonts.ready is available, we should just wait however long it takes, it may very well take a second, and since this is only the first render, it should be fine. Otherwise, we would wait 0.5s and then run into the same issues this solves.
When document.fonts.ready is not available, we would always wait the timeout, even if the fonts are actually cached, so for most renders it would slow things down unnecessarily. Looks like it's well supported anyway.
assets/packs/data_table/Skeleton.js
Outdated
| <thead> | ||
| <tr class="bg-slate-50 h-[1.5rem]"> | ||
| ${createHeaderCell("w-12")} | ||
| ${createHeaderCell("w-32")} | ||
| ${createHeaderCell("w-48")} | ||
| ${createHeaderCell("w-20")} | ||
| </tr> | ||
| </thead> |
There was a problem hiding this comment.
I think it's fine to skip the header, the difference from rows is very subtle, and since it's a placeholder it's barely noticeable anyway.
|
@jonatanklosko I see you already made the suggested changes, thanks! I tested again with the new changes, and it's working for me in Chrome and Firefox. Should we merge that? |
|
Thanks! |
|
@jonatanklosko and whenever we can release a new kino, so that users could benefit from that improvement, it would be great. Also, now that this works for Chrome and Firefox, do you have ideas on how to make it work for Safari? I tried a few different things but could not make it work properly. |
2 participants
Context
When rendering a
Kino.Datable, I encountered two issues:1. Table header does not fit within the width
2. Fonts are applied only after hovering
CleanShot.2025-03-04.at.15.06.02.mp4
This PR resolves problem 1 for Chrome and Firefox.
Before
before.mp4
After
after.mp4