getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
213 stars 48 forks source link

Loader with aggregation and visual loading state #555

Closed davidlatwe closed 3 years ago

davidlatwe commented 4 years ago

This PR supersede and will close #553. The problem in #533 was, quoting from https://github.com/getavalon/core/pull/553#issuecomment-643961476:

One thing to be noted, this PR lists out all subsets then fetching versions later, so if there is subset that has no versions at all, it will remain in view. Although this is a rare case in production but better to avoid that. The another approach I am currently trying will not have this issue.

And this is the PR what was promised in that comment.

What's changed

And here's the visual result. Note that I am not in the office at the moment, so I mocked the loading time in order to see the loading spinner in view.

a2465c4d-5e79-4992-af0d-cd52b11e3522

And sorry for Pype guys @mkolar, @iLLiCiTiT , need another review from you and test if this PR will cause any trouble to merge #550.

Please let me know what you think !

davidlatwe commented 4 years ago

Hmmm, just had a stab on merging #550, seems not an easy task.

iLLiCiTiT commented 4 years ago

Logic seems to be ok for me. I'll have to modify it to be able to test it in our fork, but at this moment I don't see any possibly breaking part. Notes I have are nice to have changes, nothing will happen if they're not accepted.

iLLiCiTiT commented 4 years ago

I can confirm it works with current multiselection implementation.

davidlatwe commented 4 years ago

I can confirm it works with current multiselection implementation.

Nice ! 🎉

BigRoy commented 3 years ago

Should we get this PR ready for merging? I imagine @davidlatwe you've been using this in production since. And likely @iLLiCiTiT adopted it in the meantime too?

iLLiCiTiT commented 3 years ago

And likely @iLLiCiTiT adopted it in the meantime too?

Yes, we use it.

mottosso commented 3 years ago

Hope you're not waiting for me, please go ahead and merge if it's ready!

BigRoy commented 3 years ago

Tested this and worked flawlessly here. @davidlatwe feel free to merge - or I'll merge in a few days.

davidlatwe commented 3 years ago

Merging this !!