Skip to content

Conversation

@lorefnon
Copy link
Contributor

@lorefnon lorefnon commented Jan 5, 2024

This PR enables the following:

  1. We can add an ?embed query parameter in the url to skip the shell container
  2. We can add a row level embed=somepage.sql parameter to lazy-load fragments of external content into cards
  3. The js behavior is modified to support interactive components in lazy-loaded fragments

While 1 is useful in itself, for embedding externally generated html content within SQLPage rendered content, it is especially useful in conjunction with 2 because we can now render output of other pages including charts, maps etc. and embed them within cards.

@lovasoa
Copy link
Collaborator

lovasoa commented Jan 5, 2024

Wow, the results look great ! This is a useful feature, and seems easy enough to use. We will merge this.
But I have two small concerns:

  • embed is a short keyword, there may be existing sqlpage applications out there using it as a URL parameter, which will make them render improperly. If we go with this design, then I would use something like _sqlpage_embed, and add somewhere to the documentation that _sqlpage_* url parameters are reserved.
  • manually implementing fragments implies quite some complexity to make everything work. Would it be a problem to simply use iframes and let the browser do the work ? Are there problems that I don't see ?

@lorefnon
Copy link
Contributor Author

lorefnon commented Jan 5, 2024

Hi @lovasoa - thanks for taking a look.

Wrt. first point I don't mind changing the param.

Wrt. second - I don't believe iframes are an ideal solution. Because of the kind of strong sandboxing they offer, they are heavyweight and each frame creates its own dom tree, separate js context etc.

The kind of use case I am currently prototyping has a few levels of drill down and for each level I'll have a grid of a dozen or so small charts in a page. So this means tabler css, apexcharts js etc. will be parsed and evaluated six times which is somewhat wasteful.

Also any change in filters etc. in sqlpage currently results in a full page refresh, so these frames will be destroyed and created again and again for every click.

I have worked with an old reporting solution in past that heavily used iframes and it was not a great experience for users.

If you think that needing to manage fragments makes the js side of things more complex then one option is to use a library like htmx or unpoly that makes swapping in dom fragments and interacting with them easier.

But yeah, it could entirely be possible that I am stretching sqlpage a bit too far from its intended use case. Let me know if you think that this may cause issues for you wrt. long term maintenance overhead. Based on your response I'll reevaluate what works best for my dashboard.

@lorefnon
Copy link
Contributor Author

lorefnon commented Jan 5, 2024

Also I don't mind adding support for frames in addition to inline html. Perhaps something like embed_mode="inline" vs embed_mode="iframe" - this could be useful for eg. if someone is embedded external content that does actually need sandboxing or style isolation.

@lovasoa
Copy link
Collaborator

lovasoa commented Jan 5, 2024

The argument of avoiding the overhead in case of a large number of frames is valid.
Indeed, having both options would be ideal, but this can be done in a second, separate, pull request.

I'm ready to merge this already if you just fix the CI error and rename the URL parameter.

@lorefnon lorefnon force-pushed the feat-card-remote-fragment-support branch from 6854de3 to 77009ac Compare January 5, 2024 20:17
@lorefnon
Copy link
Contributor Author

lorefnon commented Jan 5, 2024

Ok, great. I have updated the query param, and the CI passes now.

@lorefnon lorefnon force-pushed the feat-card-remote-fragment-support branch from b1f2b5b to 814ffdc Compare January 5, 2024 20:54
Copy link
Collaborator

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

Thanks again, this is a great addition to sqlpage

@lovasoa lovasoa merged commit 99c7e02 into sqlpage:main Jan 5, 2024
@lovasoa
Copy link
Collaborator

lovasoa commented Jan 28, 2024

I hadn't been careful, but it looks like this broke support for the height property of the chart component. All charts are now way too large.

lovasoa added a commit that referenced this pull request Jan 28, 2024
fixes bug introduced in #175
@lorefnon
Copy link
Contributor Author

lorefnon commented Jan 29, 2024 via email

@lovasoa
Copy link
Collaborator

lovasoa commented Jan 29, 2024

thanks @lorefnon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants