Skip to content
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

Conversation

KevinFabre-ods
Copy link
Contributor

@KevinFabre-ods KevinFabre-ods commented Jul 1, 2024

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

  • Description is complete
  • Commits respect the Conventional Commits Specification
  • 2 reviewers (1 if trivial)
  • Tests coverage has improved
  • Code is ready for a release on NPM

@KevinFabre-ods KevinFabre-ods added the enhancement New feature or request label Jul 1, 2024
@KevinFabre-ods
Copy link
Contributor Author

👋 @etienneburdet and @cybervinvin

With our last PRs being merged, this PR is now ready to be reviewed.

Copy link
Contributor

@etienneburdet etienneburdet left a 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.

Comment on lines 54 to 61
<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}
Copy link
Contributor

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:

Suggested change
<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.

Copy link
Contributor Author

@KevinFabre-ods KevinFabre-ods Jul 15, 2024

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

packages/visualizations/src/components/Table/types.ts Outdated Show resolved Hide resolved
@KevinFabre-ods
Copy link
Contributor Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants