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

Img Lazy Load Changes #171

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Img Lazy Load Changes #171

wants to merge 5 commits into from

Conversation

wratho
Copy link
Contributor

@wratho wratho commented Jan 26, 2018

I do not know if the base use case of lazy loading was intended to support failed image loading but i think it's a useful feature. I was going to add an example, but i couldn't feel confident in what i was adding to the template, so that might be something that needs to be added later.

The class is intended to stop the background/animation on the wrapper after the images fail to load so it's not running forever on top of the potential failed image.

as usual, modifications to this are welcome, and if anything is unclear please let me know.

added option for error image source
added the jQuery items to make it play nice with angular
referenced 'errored' class when image fails to load
when an image completely fails to load it should stop the flowy gradient transition. If you had an error source that image should also cover the entire area so that it also covers the gradient transition.
moved errored class to the wrapper level to avoid image ratio warping
moved errored class to wrapper level to allow the class to remove the animation and background without warping the images aspect ratio.
Copy link
Contributor

@kendrick kendrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like introducing some graceful degradation here by handling onerror. I haven't been able to trigger the error condition either by disabling the network before lazy load kicks in, or by changing the data-src-lg… would like to walk through a test case with you just for my own curiosity.

}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brace needs to stay for the file to compile!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must have missed this when i copied my changes over from notepad++

transition: opacity 200ms ease-in-out;
}

&.errored{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's small, but can you please add a space between the selector & brace here and in the nested selector to pass the project's linting rules? (If you want I can walk you through setting up sass-lint in VS to catch this stuff earlier.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i will update this. i usually do have that space, oops

@wratho
Copy link
Contributor Author

wratho commented Jan 30, 2018

yea stop by any time and i'll show you at least how it is erroring in Horizon

updated to pass sass formatting rules
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.

2 participants