feat: add lazy component #615

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.

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

Jump to bottom

Draft

mtt-artis wants to merge 5 commits into sqlpage:main

from mtt-artis:lazy

Draft

feat: add lazy component #615

mtt-artis wants to merge 5 commits into sqlpage:main from mtt-artis:lazy

Conversation

@mtt-artis

Copy link

Contributor

@mtt-artis mtt-artis commented Sep 22, 2024

Hi 👋,

I'm working with some resource-intensive queries that take some time to execute, and the embed option on the card component works really well. However, I’d like to prevent layout shifts and avoid having cards nested inside other cards.

The lazy component 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.


 feat: add lazy component

3f0b814

@lovasoa

Copy link

Collaborator

lovasoa commented Sep 22, 2024 •

edited

Loading

Hello ! This sounds like a very useful component to add, and I'll be happy to integrate it. Thank you ! 😊

Could you please:

remove the indentation changes to make the code easier to review
add documentation for the component in /examples/official-site/sqlpage/migrations
- The audience of SQLPage is generally not familiar with web development, so we need to find a way to explain the concept and why it is useful in a way that is approachable to non-technical users.
add a test for it in /tests/end-to-end/official-site.spec.ts

lovasoa

lovasoa requested changes

Sep 22, 2024

View reviewed changes

sqlpage/sqlpage.js Outdated

if (c.ariaBusy === "true") return;

c.ariaBusy = true;

const [source, params] = c.dataset.embed.split("?");

if (!source) return;

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

Here you may want to log an error, but still continue with the other lazy elements on the page

Copy link

Contributor Author

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

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

my bad, rest of a forEach variant

sqlpage/sqlpage.js Outdated

Comment on lines 13 to 15

if (!search.has("_sqlpage_embed")) {

search.set("_sqlpage_embed", "true");

}

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

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("?");

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

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]")) {

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

Maybe this should be done in parallel rather than sequentially ?

Copy link

Contributor Author

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

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

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.

fix

7cea552

@mtt-artis

Copy link

Contributor Author

mtt-artis commented Sep 22, 2024

hi 👋,
Thanks for your comments

remove the indentation changes to make the code easier to review

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 😉

mtt-artis

mtt-artis commented

Sep 22, 2024

View reviewed changes

examples/cards-with-remote-content/lazy.sql

select 'lazy' as component;

select

'/chart-example.sql?_sqlpage_embed' as embed,

Copy link

Contributor Author

@mtt-artis mtt-artis Sep 22, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

should we keep as embed or as lazy here ?

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

I think as embed is good

@lovasoa

Copy link

Collaborator

lovasoa commented Sep 22, 2024

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.


 style: revert indent on sqlpage.js

55f368b

lovasoa

lovasoa reviewed

Sep 22, 2024

View reviewed changes

sqlpage/templates/lazy.handlebars

@@ -0,0 +1,13 @@

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

In all other components, class adds new classes without replacing the existing ones

Copy link

Contributor Author

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

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

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 ?

lovasoa

lovasoa reviewed

Sep 22, 2024

View reviewed changes

sqlpage/sqlpage.js Outdated Show resolved Hide resolved

mtt-artis

mtt-artis commented

Sep 22, 2024

View reviewed changes

sqlpage/sqlpage.js Outdated

c.dispatchEvent(fragLoadedEvt);

})

function sqlpage_embed() {

for (const c of document.querySelectorAll("[data-embed]")) {

Copy link

Contributor Author

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

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

is the query better ?
document.querySelectorAll("[data-embed]:not([aria-busy=true])")

Copy link

Collaborator

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

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

Yes it's better !

@mtt-artis @lovasoa


 fix: update sqlpage.js

182ab38

Co-authored-by: Ophir LOJKINE <contact@ophir.dev>

@lovasoa lovasoa mentioned this pull request

Sep 22, 2024

Carousel component - Support for embedding other sql files #605

Open

@mtt-artis


 refacto: add ':not([aria-busy=true])'

33a2c06

@lovasoa

Copy link

Collaborator

lovasoa commented Oct 19, 2024

Hello ! Any news on this ? I think only docs and tests are missing, then we can merge !

Labels

None yet

2 participants

@mtt-artis @lovasoa

Uh oh!

feat: add lazy component #615

Are you sure you want to change the base?

feat: add lazy component #615

Uh oh!

Conversation

@mtt-artis mtt-artis commented Sep 22, 2024

Uh oh!

lovasoa commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

mtt-artis commented Sep 22, 2024

Uh oh!

@mtt-artis mtt-artis Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

lovasoa commented Sep 22, 2024

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@mtt-artis mtt-artis Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

@lovasoa lovasoa Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

lovasoa commented Oct 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lovasoa commented Sep 22, 2024 •

edited

Loading

@mtt-artis mtt-artis Sep 22, 2024 •

edited

Loading