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

Feature/lazyloading #119

Merged
merged 24 commits into from Apr 19, 2018
Merged

Feature/lazyloading #119

merged 24 commits into from Apr 19, 2018

Conversation

robertvrabel
Copy link
Contributor

@robertvrabel robertvrabel commented Apr 17, 2018

Leaving this open for a bit as I want to discuss it with the team.

  • This adds a polyfill for IntersectionObserver which figures out what images are in bound in the users viewport and lazy loads them to save bandwidth.
  • Gets rid of the old b-lazy so everything uses IntersectionObserver
  • Images from factories are now unique by number so in the network tab of the page it counts as a new image to load. This is helpful when testing lazy loading situtations
  • The 1px gif is now located in one place at partials.image-fake so its easier to use
  • Split the image list page in the styleguide so its Image List Rotate and Image Button List
  • Skip links are now it's own partial SCSS file instead of being grouped in the utilities file
  • Adds a custom blade directive for including a lazy image: @image('path-to-image', 'alt text', 'optional-class-name')

@robertvrabel robertvrabel requested a review from a team April 17, 2018 18:58
@coveralls
Copy link

coveralls commented Apr 17, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5154103 on feature/lazyloading into 09092f0 on develop.

let lazyImage = entry.target;

if(lazyImage.tagName != 'IMG') {
lazyImage.style = "background-image: url('"+lazyImage.dataset.src+"')";
Copy link
Member

Choose a reason for hiding this comment

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

Would this overwrite any other inline styles on the image?

Feel like this may be a better approach to swap out the background-image:

lazyImage.style.backgroundImage = "url('"+lazyImage.dataset.src+"')";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea i'm good with doing that! I'll make the change.

Copy link
Contributor Author

@robertvrabel robertvrabel left a comment

Choose a reason for hiding this comment

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

I added a custom blade directive for an image @image('path-to-image', 'alt text', 'optional-class-name')

Its wonky how you pass in multiple optional parameters, but I got the idea from this to split the string: https://zaengle.com/blog/exploring-laravels-custom-blade-directives . I'm looping through to check how many params were passed in and then I set them as blank strings (so they are optional) if not passed in.

let lazyImage = entry.target;

if(lazyImage.tagName != 'IMG') {
lazyImage.style = "background-image: url('"+lazyImage.dataset.src+"')";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea i'm good with doing that! I'll make the change.

@robertvrabel robertvrabel merged commit 5154103 into develop Apr 19, 2018
@robertvrabel robertvrabel deleted the feature/lazyloading branch April 19, 2018 15:00
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.

None yet

3 participants