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

Move the image in g-image into a <picture> tag and fade in on load #1460

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hjvedvik
Copy link
Member

@hjvedvik hjvedvik commented Mar 12, 2021

This PR moves the image in the g-image component into a picture element and fades it in when loaded. So the new markup looks like this:

<div class="g-image g-image--lazy g-image--loading">
  <div style="width:100%;padding-bottom:67%;"></div>
  <img class="g-image__placeholder" src="data:image/png;base64,..." width="480" height="320" aria-hidden="true">
  <picture class="g-image__picture">
    <source type="image/png" srcset="image.png 480w">
    <img class="g-image__image" src="image.png" width="480" height="320" alt="...">
  </picture>
</div>
<noscript>...</noscript>

This change will also make it possible to add support for WebP images which fallbacks to Jpeg.

Closes #212

@hjvedvik hjvedvik self-assigned this Mar 12, 2021
@u12206050
Copy link
Contributor

Not to sure about this even though both fade in and webp fallback is nice to have.

  1. Should it be up to the developer how the image 'appears'?
  2. What if image type is handled by the an image cdn or some other logic?

Another issue is that image width and height isn't always passed, so the calculation you have will most likely not work.

SUGGESTION: Add it as a separate g-picture instead?

@hjvedvik
Copy link
Member Author

@u12206050 I'm not sure if it is necessary to have two separate components for serving images. We can add a slot to allow developers to override the output in another PR if people want it.

The main purpose of g-image is to minify and serve local images. But we do want to make it easier to serve external images with it by adding more meaningful props instead of sending an object into the src property (which is what local images do behind the scene). So it will look something like this:

<g-image
  width="1920"
  height="1080"
  alt="An image ..."
  src="https://cdn.example.com/image.png"
  placeholder="data:image/png;base64,..." <!-- Can also be a URL to a tiny image -->
  :sources="[{ type: 'image/png', srcset: '...' }, ...]"
/>

While width and height attributes are set automatically for local images, they must be set manually for external images to maintain the aspect ratio with the new markup.

@ramsesgarate
Copy link

@hjvedvik This has not been approved yet? :-(

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.

Use <picture> for g-image
3 participants