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.

Sign up for GitHub

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

Jump to bottom

Re-enable No JS Fallback #19

Open
techwes opened this issue Nov 28, 2018 · 4 comments
Open

Re-enable No JS Fallback #19

techwes opened this issue Nov 28, 2018 · 4 comments

Comments

@techwes
Copy link

techwes commented Nov 28, 2018

Hello! First off I am a new user of react-lazy-images and want to thank you for your great work here!

Based on this comment and my personal experience using a noscript fallback in production (check out the Featured Venue images on this page with JS off) it seems like React has fixed the issue regarding using <noscript>. The code mentions that when this issue is fixed this library would once again offer a fallback API.

I have no problems with how this was originally implemented and documented. I understand this really is not a priority since it is easy to implement along side this module but figured I would bring it up to put it on your radar. I may also be able to find time to implement this and submit a pull request if that works better for you.
@fpapado
Copy link
Owner

fpapado commented Dec 19, 2018

Hi @techwes!

Sorry for taking a while to get back to you, it's been a hectic few weeks.

Re-enabling the noscript fallback has been on my mind for a while.
I also use a "manual" noscript in production with the more recent React versions and it's been great!
Part of making this library was to have a good, resilient lazy-loading implementation, and the noscript is an important part of that.

Things to consider

Versioning

There is a small dilemma here:

  • Older React versions will double-fetch images (potentially of different sizes if you use srcset)
  • New React versions work correctly, and noscript is useful

One potential solution would be to release a new minor version, that bumps the peerDependencies entry to > 16.5.0, and adds the noscript API as well.
The thing I don't like there is that, say, 16.4 is still usable, except for the fallback. There is no good way to communicate that with dependencies afaict.

noscript API

Another thing is that the original API was relying on the user to match styles between the actual and noscript. We should consider if there is a better alternative. Would you be able to share a snippet of the noscript in your use case? That could be a good data point in addition to the one I have!

Preact?

When I tested it a few months back, Preact seemed ok with noscript. Double-checking wouldn't hurt still.

Timing

Since it's the holidays I have some more time on my hands and could look into this more. Thanks for bringing it up as you did, it is super helpful :)

@techwes
Copy link
Author

techwes commented Jan 3, 2019

Hello thank you for getting back to me and I apologize for the delay in my response. The holidays were a very busy time for me this year. To start below is my thin wrapper around LazyImage that I use:

import React, { Fragment } from 'react';
import { LazyImage } from 'react-lazy-images';
import classNames from 'classnames';

const placeholder = ({ imageProps, ref }) => (
  <div
    ref={ref}
    className={classNames(imageProps.className, 'u-imgPlaceholder')}
  />
);

const actual = ({ imageProps }) => <img {...imageProps} />;

const LazyImg = props => {
  return (
    <Fragment>
      <LazyImage {...props} actual={actual} placeholder={placeholder} />
      <noscript>
        <img {...props} />
      </noscript>
    </Fragment>
  );
};

export default LazyImg;

In order to make sure the placeholder does not take up any space in the layout I have this css rule:

.no-js .u-imgPlaceholder {
  display: none;
}

Where no-js is a class that starts on the html element and gets removed via a script tag in the head (I already had this implemented for some other fallbacks).

I think the library could call the actual function inside the <noscript> to ensure the fallback matches what would have shown up exactly. As for making sure the placeholder does not show, that is a little harder. You could add an inline display: none on server render and then remove it on componentDidMount but that will probably not work well. I think most people who do SSR (including me) put their JS loading at the end of the <body> which will mean (depending on how long the JS takes to load) the page will show with no image or placeholder which is not the point of the placeholder.

As far as versioning that is very tricky indeed. I am no expert here and am not sure what makes the most sense. You could split into two different modules, one with the fallback and one without but then you would have to add other new features to both and that doesn't seem worth it.

It may end up being best to have the fallback as a separate add-on module so it can be conditionally included for those who need it and use a React version that supports it. Since we have both implemented it without access to any of the library internals, this is most likely very possible.

@VitalyKrenel
Copy link

Hey @fpapado! Have you settled on any final decision yet with Fallback component?

Right now things are slightly confusing for new clients of the library: they need to investigate on their own what's the issue with noscript you were talking about - readme covers the problem but there is a lot stuff needed for new client to process - and also understand what they can do to overcome the problem.

I can submit a PR with Readme update if needed.

@aralroca
Copy link

Please, activate again this 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@techwes @fpapado @aralroca @VitalyKrenel