Fixes loading states in general

[nucleogenesis]

Summary

A handful of fixes to various loading states in Learn, smoothing things out a bit.

Overall, the LearnAppBarPage was taking a loading prop but it only got that prop from two components that use it. Ultimately, I think that should have never been a prop and should just be set at the highest level to reflect the value of the Core loading state.

The result now is that when we load content of a page initially, we show the KLinearLoader in the AppBar -- per the KDS loaders guidelines -- but then use the circular loaders when loading new information.

I also removed and updated logic around the loaders on the BookmarkPage where now the circular loader only shows when "loading more" to the same effect.

References

Fixes #10336

Reviewer guidance

Note that some of these things may be easier to observe with network throttling.

1. User table shows loader while fetching users

users-loader.mp4

2. Lesson playlist page does not show "no resources" message while fetching, app bar linear loader is active

lessonplaylistpage.mp4

3. Flash in the Topics Page navigation

topics-page.mp4

---

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a14.dev0_git.68.ge67a738c.pex
Windows Installer (EXE)	kolibri-0.16.0a14.dev0+git.68.ge67a738c-unsigned.exe
Debian Package	kolibri_0.16.0a14.dev0+git.68.ge67a738c-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a14.dev0+git.68.ge67a738c-0.3.0.dmg
Source Tarball	kolibri-0.16.0a14.dev0+git.68.ge67a738c.tar.gz
WHL file	kolibri-0.16.0a14.dev0+git.68.ge67a738c-py2.py3-none-any.whl

[marcellamaki]

@nucleogenesis - I've started a code read through and this looks good. But I do just want to note that the original scope of the issue was across the whole frontend, not just in learn. If it would be helpful, I can create follow up issues to address this in the rest of the plugins, so they could be split up? Let me know what you prefer.

@radinamatic - some robust manual QA would be helpful here, for sure, although I'll be doing some as well.

[nucleogenesis]

@marcellamaki I did dig through the other plugins but didn't note any other spots where I saw no loading states.

I didn't do anything on change facility or setup wizard though so can double check there.

If you have any specific spots where this has been reported I'm happy to add those onto this PR.

[marcellamaki]

Ah, yes, sure! I've been noticing it in coach, mostly because I've been working in there a bit recently, but an initial response from @pcenov doing some replication this indicate it is fairly widespread at least in Learn and Coach, although I suppose it maybe have been resolved incidentally in the interim? I'm not sure about the device and facility plugins - it may not be as noticeable there also because of the types of pages and interactions.

He shared this with @radinamatic

My takeaway is more that this is not a matter of QA'ing to find a handful of places where it is missing, since it is inconsistently implemented. I can help with some replication later this week of that would be helpful!

[marcellamaki]

Thanks for continuing the work on this @nucleogenesis! I can see that you've dug into a lot of things here, and that you're really working across a lot of the plugins to get things resolved.

My main issue with this is although it may work more smoothly (I haven't done robust manual QA yet, but I can already tell from the code that there will be improvements), the loading state still seems inconsistent and a bit confusing while reading through.

For example - in the AppBarPage, you've added v-if="!$store.state.core.loading" on the main-wrapper div, which makes a lot of sense to me. But there's still a loading prop, and the KLinearLoader is visible based on that prop.

Within FacilityAppBarPage you are passing:

<AppBarPage :title="title" :loading="$store.state.core.loading">

My suspicion is that this combination of "what is loading and where is it coming from" is just an oversight from refactoring, but I think it's worth cleaning up now, across these files and making sure the same type of thing isn't popping up in other places. I can see myself coming to this code later and wondering "why both? maybe the prop isn't even passed in? has it been deleted in the parent and now it's just undefined and not doing anything?"

I can see how tangled up a lot of this is (and you've alluded it it a few times during standups 😄) and I don't want to send you too far down a rabbit hole. I also know your feelings about Vuex! But, I think having a secondary goal of a consistent pattern for using the loading state(s) -- and documenting it, even if only in a few code comments in the most high level shared page components -- would be really helpful to our future selves. I suspect you are already working towards identifying and implementing that consistent pattern, but writing it out might be just the reference point you need to go back through and ensure consistency within and across plugins and pages.

I'm happy to talk more about this and brainstorm some ideas with you and the pros and cons of approaches. I think it's fine to use both Vuex and props, but in my opinion, it's confusing to use both in the same component. Something like the vuex in the highest parent component, and then passing down the props to other child components as needed could work, and might eliminate the need to be using vuex everywhere. I could imagine there might be cases where that won't work though, or where it might be a little more complex.

Let me know your thoughts on this and if you'd want to talk though anything more. Another option might be to split this into several issues and once we establish a consistent pattern, divide up the work per-plugin so that it doesn't all have to be on you.

Thanks again for the ongoing efforts here - I know this is not an easy problem to sort out.

[nucleogenesis]

@marcellamaki

I can see myself coming to this code later and wondering "why both? maybe the prop isn't even passed in? has it been deleted in the parent and now it's just undefined and not doing anything?"

Ahh yeah - I meant to use the $store.state.core.loading value as the primary indicator of "something is loading" throughout.

So in other changes I've made, I used that value to toggle circular loaders.

It honestly feels weird that the whole app has one "something is loading" state value like we could run into a race condition if we are loading multiple things simultaneously if they all are setting/unsetting the loading state.

In the cases where I use it it seems to work alright but it still feels odd.

---

In any case - I think that using something like mapState({ loading: state => state.core.loading}) wherever we need the value will work. It doesn't look like there are many places even passing the loading prop up to the various AppBar pages and so it shouldn't be too much to clean all that up and include it here. I'll re-request code review when done.

[nucleogenesis]

@pcenov hey! I've added before/after videos (both are in the same video) for each of the main fixes here. I'll add some more changes, but none that will require re-review

[pcenov]

Hi @nucleogenesis,
I'm not able to verify your first point 'User table shows loader while fetching users' as currently there's a problem with the Users page as none of the newly created users get displayed in the table - so something is broken there:

nousers.mp4

The other two points are implemented as specified in the reviewer guidance and demonstrated by your videos.

I have also recorded two videos, the first one as a Learner and the second one as a Super admin interacting with Kolibri while the Slow 3g throttling is on. This is just for illustration that we have 3 different loading indicators - the Kolibri logo, a spinner and a green line under the top bar and in some cases no indication at all. So to me it seems that areas that can benefit from additional improvements would be the sign-in page where there is no loading indicator, everything in the Device and Coach plugins where there's also no loading indicator, viewing a Quiz as a learner for the first time and anything else that doesn't clearly indicate that things are loading.

Learner:

learner.interaction.mp4

Super admin:

admin.interaction.mp4

[nucleogenesis]

Just noting that I'm working on updating to resolve the conflicts. I'll be moving on to complete @pcenov 's listed issues above after I determine whether or not I'm causing the Users table to not populate with newly created users.

Will aim to have this ready for re-review after tomorrow

[marcellamaki]

@nucleogenesis - just confirming I see the update loader state in the user table. I did a little bit of manual QA - not as robust as @pcenov of course :D - but initially it does seem to be good, although Peter's point about the multiple loading states is well taken. This should certainly be something we discuss for an upcoming release.

Jacob, let me know when this is ready for final code review and I'll take a look!

[nucleogenesis]

@marcellamaki I went to revise a few things to clean things up on the "one or the other" front re: Vuex vs Component state and such -- I opted to put the loading states for discrete areas where things might be loading into more local Vuex module. This way you can introspect not just if something is loading, but whether a specific piece of data is loading.

This brought with it some non-comprehensive refactors (as in, there are more places where this can be applied). I figure that getting review on it where it is now would be good and I can follow-up with a list of other places where this pattern can be applied.

For code review, I suggest going commit-by-commit as in most cases they should represent a self-contained change.

[nucleogenesis]

Also, this fixes user tables broadly where it will no longer show "No users to show" while loading. So it would be helpful if @pcenov you could check the user tables in:

• facility -> users
• facility -> classes -> class management
• facility -> classes -> class editing
• facility -> permissions (see video - this video's fixes should be similar in all instances of the "user table")

fix--moar-loading-states-4453a9183f.mp4

• coach -> group edit/create
• coach -> plan -> lesson assignment table
• device -> channels list (see video)

fix--moar-loading-states-0090bf15f.mp4

[marcellamaki]

Thanks @nucleogenesis - it seems like critical feedback has been addressed and that there is a general pattern that we can build and extend on this pattern, so I feel okay about merging this and building on it in follow up.

One essential point of immediate follow up however, is that the loading state is not well managed in remote content browsing, whereas the improvements in the rest of learn are pretty clear. I'm not sure why it's different than the current library page, but I imagine it's related to the remote-browsing-ness. I would really like for us to work to get this resolved, and it isn't clearly covered by existing issues.

Here's a short video example of what I'm talking about, but overall, I think if you explore the remote browsing feature with a throttled connection (or even a not throttled connection - this was just for it to be well captured in screengrab) you will see that there is a lot of sort of.... cached + overriding situations happening that would be good to smooth out.

If you have a chance to open an issue, that would be great. Otherwise, I will open one tomorrow.

remote-content-loading.mp4