Conversation
|
Hello ! This sounds like a very useful component to add, and I'll be happy to integrate it. Thank you ! 😊 Could you please:
|
sqlpage/sqlpage.js
Outdated
| if (c.ariaBusy === "true") return; | ||
| c.ariaBusy = true; | ||
| const [source, params] = c.dataset.embed.split("?"); | ||
| if (!source) return; |
There was a problem hiding this comment.
Here you may want to log an error, but still continue with the other lazy elements on the page
There was a problem hiding this comment.
my bad, rest of a forEach variant
sqlpage/sqlpage.js
Outdated
| if (!search.has("_sqlpage_embed")) { | ||
| search.set("_sqlpage_embed", "true"); | ||
| } |
There was a problem hiding this comment.
maybe just search.set("_sqlpage_embed", ""); without the if ?
sqlpage/sqlpage.js
Outdated
| for (const c of document.querySelectorAll("[data-embed]")) { | ||
| if (c.ariaBusy === "true") return; | ||
| c.ariaBusy = true; | ||
| const [source, params] = c.dataset.embed.split("?"); |
There was a problem hiding this comment.
We can parse the url correctly with new URL(c.dataset.embed, window.location.href)
sqlpage/sqlpage.js
Outdated
| c.dispatchEvent(fragLoadedEvt); | ||
| }) | ||
| function sqlpage_embed() { | ||
| for (const c of document.querySelectorAll("[data-embed]")) { |
There was a problem hiding this comment.
Maybe this should be done in parallel rather than sequentially ?
There was a problem hiding this comment.
I am not sure if I understand you.
We don't await the fetch so the loop does't wait the promise to be resolved to start the next element.
|
hi 👋,
could we invest for future change and make the indentation consistent across the file ? I can add tests and documentation during the next WE if you don't mind 😉 |
| select 'lazy' as component; | ||
|
|
||
| select | ||
| '/chart-example.sql?_sqlpage_embed' as embed, |
There was a problem hiding this comment.
should we keep as embed or as lazy here ?
|
Yes, we can set an indentation standard, but maybe after your PR? You are going to end up with large conflicts for nothing, otherwise. I need a clean diff in this PR to review it. |
| @@ -0,0 +1,13 @@ | |||
| <div class="{{default class "my-2"}}"{{~#if style}} style="{{style}}" {{/if~}}> | |||
There was a problem hiding this comment.
In all other components, class adds new classes without replacing the existing ones
There was a problem hiding this comment.
i know and i hesitate, this div is a light wrapper and i think user should be able to reset it easily if needed.
same for <div class="{{default class "card my-2"}}">. i add this default because most of the component are card.
should we just give no default and handle it like the style param ?
sqlpage/sqlpage.js
Outdated
| c.dispatchEvent(fragLoadedEvt); | ||
| }) | ||
| function sqlpage_embed() { | ||
| for (const c of document.querySelectorAll("[data-embed]")) { |
There was a problem hiding this comment.
is the query better ?
document.querySelectorAll("[data-embed]:not([aria-busy=true])")
Co-authored-by: Ophir LOJKINE <contact@ophir.dev>
|
Hello ! Any news on this ? I think only docs and tests are missing, then we can merge ! |
2 participants
Hi 👋,
I'm working with some resource-intensive queries that take some time to execute, and the
embedoption on thecardcomponent works really well. However, I’d like to prevent layout shifts and avoid having cards nested inside other cards.The
lazycomponent follows a similar logic but provides an element you can style before it's replaced with the actual content.Please feel free to make any changes, rename the component (coming from React/Solid, that's what comes naturally to me), or guide me in the right direction.
If you like this implementation, I'd be happy to add it to the documentation.