Closed lorefnon closed 6 months ago
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.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.
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.
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.
Ok, great. I have updated the query param, and the CI passes now.
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.
Yes, sorry about that. This fix is not ideal though. I'll send a PR.
On Mon, 29 Jan, 2024, 1:51 am Ophir LOJKINE, @.***> wrote:
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.
— Reply to this email directly, view it on GitHub https://github.com/lovasoa/SQLpage/pull/175#issuecomment-1913711019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALB4FCXDPHL5CPYKIZGDGLYQ2XMXAVCNFSM6AAAAABBOWP7BKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTG4YTCMBRHE . You are receiving this because you authored the thread.Message ID: @.***>
thanks @lorefnon !
This PR enables the following:
?embed
query parameter in the url to skip the shell containerembed=somepage.sql
parameter to lazy-load fragments of external content into cardsWhile 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.