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

Bugfix/lazy component loading states and fetching #2500

Conversation

filrak
Copy link
Collaborator

@filrak filrak commented Feb 25, 2019

Short description and why it's useful

  • added ability to reload component when fetch fails
  • added loading states for async sidebar components
  • added error handling for offline/online errors

TODO

  • add left sidebar support
  • add custom width support
  • refactor transitions
  • add close button on failed load
  • preload crucial async components
  • add translation keys

Screenshots of visual changes before/after (if there are any)

(Failed loading - is offline)
image
(Failed loading - is online)
image
Component is loading
image

The solution is still dirty and needs polishment. Just leaving the WIP PR to mostly show screenshots :)

@filrak filrak changed the title wip: Bugfix/lazy component loading states and fetching WIP: Bugfix/lazy component loading states and fetching Feb 25, 2019
@pkarw
Copy link
Collaborator

pkarw commented Feb 25, 2019

This is pretty important fix and should be applied even when not polished. However - the Micorcart, Menu + Wishlist sidebars should be eagerly loaded for the offline mode - this is something i discussed with @patzick and there is a ticket for that. So I'd preffer to have them eagerly loaded this week and then think off what's left for this fancy loader You created here @filrak - we should have both!

@filrak
Copy link
Collaborator Author

filrak commented Feb 25, 2019

It will be finished today. With this approach user will be able to refresh failed component just with one click (we can even try to do this automatically after connection is restored).

I'm not that a big fan of eager loading of this components. No matter if we have eager load or prefetch they still need to be downloaded anyway and in uncertain network conditions the bigger initial bundle is the worse for us.

What is the exact reason of eager load of this offscreen components?

@patzick ?

@pkarw
Copy link
Collaborator

pkarw commented Feb 25, 2019

Go to the product page, switch the internet off, add the product to the cart -try checking the cart status :) Good luck; at least for now ;)

That's the reason for eagerly loading the key components - just cart, wishlist + navigation.

@filrak

@filrak
Copy link
Collaborator Author

filrak commented Feb 25, 2019

Ok, then this PR is prty useless in core functionalities. More like an additional feature

@pkarw
Copy link
Collaborator

pkarw commented Feb 25, 2019

We should have it + Microcart + navigation eagerly loaded. I really like this feature - let's merge it in

@filrak
Copy link
Collaborator Author

filrak commented Feb 26, 2019

I added automatic reload of failed components when user cames back online

@pkarw
Copy link
Collaborator

pkarw commented Feb 26, 2019

Ok; fine with me

@filrak
Copy link
Collaborator Author

filrak commented Feb 26, 2019

Done. I will improve some parts during stability phase (mostly purge unused css in in-sidebar components and props allowing to specify width of sidebar) but for now it works perfectly fine and solves the problem gracefully ;).

@filrak filrak changed the title WIP: Bugfix/lazy component loading states and fetching Bugfix/lazy component loading states and fetching Feb 26, 2019
@filrak filrak requested a review from patzick February 26, 2019 14:18
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Really cool feature!
I'd just set default values for component props and changed passing functions as props to vue-styled emits.

patzick and others added 12 commits February 26, 2019 19:37
…idebar.vue

Co-Authored-By: filrak <f.rakowskI@hotmail.com>
…idebar.vue

Co-Authored-By: filrak <f.rakowskI@hotmail.com>
…gError.vue

Co-Authored-By: filrak <f.rakowskI@hotmail.com>
…idebar.vue

Co-Authored-By: filrak <f.rakowskI@hotmail.com>
…gError.vue

Co-Authored-By: filrak <f.rakowskI@hotmail.com>
Co-Authored-By: filrak <f.rakowskI@hotmail.com>
Co-Authored-By: filrak <f.rakowskI@hotmail.com>
Co-Authored-By: filrak <f.rakowskI@hotmail.com>
Co-Authored-By: filrak <f.rakowskI@hotmail.com>
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

It's okay for me, we should only reference to issues #2408, #2451

CHANGELOG.md Outdated Show resolved Hide resolved
@patzick
Copy link
Collaborator

patzick commented Feb 26, 2019

CR does not link issues so i need a separate comment - related issues #2408, #2451

patzick and others added 3 commits February 26, 2019 21:03
Co-Authored-By: filrak <f.rakowskI@hotmail.com>
@patzick patzick merged commit 4f30bf4 into vuestorefront:develop Feb 26, 2019
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.

3 participants