-
Notifications
You must be signed in to change notification settings - Fork 3
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] New table column type: media #252
[FEAT] New table column type: media #252
Conversation
Render image in a tooltip on hover/tab
👋 @etienneburdet and @cybervinvin With our last PRs being merged, this PR is now ready to be reviewed. |
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.
Looking mostly good and working well. IMO we can leverage image events a bit more, which would simplify the code a bit.
packages/visualizations/src/components/Table/Cell/Format/ImageFormat.svelte
Outdated
Show resolved
Hide resolved
<div class="ods-visualization-table__image-error-container"> | ||
{@html errorContent(rawValue)} | ||
</div> | ||
{:else} | ||
<div class="ods-visualization-table__image-loading-container"> | ||
{@html loadingContent(rawValue)} | ||
</div> | ||
{/if} |
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 don't think we should use @html
to replace the lack of children in the SDK for now. It opens up a lot of security breaches (not for new Image
which is safe) and forces the user to write HTML in template strings.
IMO, we should have something like this:
<div class="ods-visualization-table__image-error-container"> | |
{@html errorContent(rawValue)} | |
</div> | |
{:else} | |
<div class="ods-visualization-table__image-loading-container"> | |
{@html loadingContent(rawValue)} | |
</div> | |
{/if} | |
<img | |
on:load={() => { isLoading = false; tippyInstance.setContent(...)}} | |
on:error={(e) => { error = e; }} | |
src={src} | |
alt={alt} | |
/> | |
{#if error} | |
<div class="ods-visualization-table__image-error-container"> | |
{error} | |
</div> | |
{/if} | |
{#if isLoading} | |
<div class="ods-visualization-table__image-loading-container"> | |
maybe some default loader | |
</div> | |
{/if} |
This leaves dom node creation to Svelte and limits errors to text and loader to pure CSS. These are limitations we should accept for now IMO.
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.
Users of this repository are developers. We are not responsible for any security breaches that may arise from the values displayed in the table. However, components APIs should be clear enough for developers to understand when they will directly change the DOM with an option.
We have a similar mechanism in place when displaying content in the MapWebGL popup (see: GitHub link).
I still simplified the options to only allow customization of the loading and error messages. 17c572d
@etienneburdet and @cybervinvin Let's manage our expectations for this PR. I'll reopen it when we have the time to work on new features for the table |
Summary
The goal for this PR is to add a new table column type:
media
(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46281.
Changes
Based on what has been initiated here, we use tippy to render a tooltip that contain an image.
Image are lazy loaded, which means that the browser request images only when the
img
tag is visible to the user.Review checklist