-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: added iframe component #237
Conversation
Looks good, thanks for taking the time to write tests and even including a screenshot for the docs ;) Great start across the different aspects of components (frontend, events, etc). I left a couple of comments, look like minor omissions. Before I merge this, can you please think about the other embeddings (e.g. PDF, Tableau, PowerBI, etc) and think a bit more about the After all, it should load the URL that the developer asked to load. I'm also a bit confused as to why we capture the URL as a string and the transform it into a Dict with only one key; you may have a plan I'm not aware of. Keen to hear your thoughts on this one. |
The rationale behind the url parameter was to enable the reuse of the handler across multiple components. However, it seems that using just the url might not suffice for this purpose, so additional information should be provided. Moreover, utilizing dictionaries makes it easier to extend the functionality in the future without causing breaking changes. |
I'd say until it's very clear what we want to do with the payload of I don't see how valuable things like URL, or other things we may want to pass (number of pages) can really be. With internals it's always good to think about expansibility but with user-facing things such as these ones we need to be extremely mindful of the complexity we're adding. |
Is there a way to have easily an event if loading the iframe went wrong ? It may be great to display a warning message and hide the iframe itself. |
Sadly, iframe only has |
Ok, removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, please address the couple of small comments and I’ll merge.
ui/package.json
Outdated
@@ -16,7 +16,7 @@ | |||
"marked": "^7.0.2", | |||
"monaco-editor": "^0.41.0", | |||
"plotly.js-dist-min": "^2.22.0", | |||
"remixicon": "latest", | |||
"remixicon": "4.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep on latest? They keep adding icons and then people want to use them, but can't find them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't think latest
keyword is valid. And when you install dependencies this way it will install version 3.5.0
which definitely is not latest
. Current version of this library is 4.1.0. If we want to have always the newest version then we should use
"remixicon": "4.1.0", | |
"remixicon": "*", |
IFrame component
I also added
ss-load
withurl
to be able to handleonload
event of iframe