-
Notifications
You must be signed in to change notification settings - Fork 805
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
Fix no resources available" instead of the available libraries #13184
Fix no resources available" instead of the available libraries #13184
Conversation
Build Artifacts
|
Tagging @radinamatic @pcenov for QA review |
Assets from this PR were tested on both Ubuntu and Windows 11, and the originally reported issue is indeed corrected. ![]() However, in the scenario that we were discussing on Slack with @rtibbles, when there are no resources in the library and no peers in the local network:
For that reason @rtibbles suggested we would want that page to be more dynamic based on whether there are local libraries available, and preserve the messaging from the content unavailable page for the empty state. So the idea is for the non-admin users to keep the current change from this PR IF there are other peers in the local network (and something does appear under the Other libraries). And in case there are no other peers around, and the class server has no channels, display the No resources available heading and Ask your coach or administrator for assistance below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m seeing the CONTENT_UNAVAILABLE
page as an admin when I should be able to browse channels on Kolibri Studio.
Based on Radina’s comment, I believe the CONTENT_UNAVAILABLE
page should only displayed for non-admin users if there are no peers in the local network and no channels.
When there are no channels and no other libraries for the learner to browse, I currently see the LibraryPage:
But in this case, a learner should see the ContentUnavailablePage with the message Ask your coach or administrator for assistance instead:
Thank you @LianaHarris360 for including the screenhots of what @rtibbles was suggesting. I thought of trying to mock that alongside my description, but it would have certainly taken me much more time! 🙏🏽 |
I think let's pull the body of this component out into a separate component: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/learn/assets/src/views/ContentUnavailablePage.vue - reuse it in the page there, and then also display it within the context of the LibraryPage.vue? That will allow us to continue to have the Content Unavailable page when we need to - but also mean we can show the appropriate language if there are no channels available, but also still display any locally available libraries (or Studio in the case of a super admin). @AllanOXDi @LianaHarris360 does that make sense as an approach here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not quite cleanly handling this empty state without side effects. I trust you are testing this with a clean Kolibri home directory to see how this renders?
The goal here is to retain our previous messaging in the case of a truly empty state, while also not blocking browsing of remote or local libraries when they are available.
@@ -0,0 +1,90 @@ | |||
<template> | |||
|
|||
<LearnAppBarPage :appBarTitle="learnString('learnLabel')"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicating the component completely - my suggestion was to make a component that represented the body of the ContentUnavailablePage component (everything but the LearnAppBarPage
component) and then reuse it in the LibraryPage and the ContentUnavailablePage.
Doing this has caused us to have two LearnAppBarPage components, one nested inside the other:
@@ -77,7 +81,7 @@ | |||
/> | |||
<!-- Other Libraries --> | |||
<OtherLibraries | |||
v-if="showOtherLibraries" | |||
v-if="showOtherLibraries && !isLearner" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
router.replace({ name: PageNames.CONTENT_UNAVAILABLE }); | ||
return; | ||
if (!channels.length) { | ||
set(isChannelEmpty, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to add this, we're already tracking this with isLocalLibraryEmpty
.
> | ||
{{ coreString('nothingInLibraryLearner') }} | ||
</p> | ||
<NoResourcePage v-if="isChannelEmpty && isLearner" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably want to conditionally show this based on whether there are any libraries around you or not. If there are, we should show the existing message below, if not, then we should show this component.
This will require us to communicate something out of the OtherLibraries component to dynamically update this based on whether any libraries are being shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rtibbles I'm not sure, do we update the view as soon as we discover a library, learner initially doesn't have local libraries on the network but then one joins? |
Yes, the OtherLibraries component is checking for other libraries on the network, so we would need a mechanism for it to communicate out when it is displaying libraries or not (probably by emitting an event and having the LibraryPage listening to that). |
import commonLearnStrings from '../commonLearnStrings'; | ||
export default { | ||
name: 'NoResourcePage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more like "NoResourceComponent" now, I think? And then we could import this into the ContentUnavailablePage to act as the body and avoid the duplication here.
}; | ||
}, | ||
components: { | ||
LearnAppBarPage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is no longer used here, hence the linting errors.
…ility of network devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be blocking or could be fixed in a follow-up, but another thing I noticed is that the NoResources
component and the OtherLibraries
message 'No other libraries around you right now' are displayed before the correct components load, with no indication that the page is still loading. This is more noticeable at slower data speeds.
LibraryPageLoad.mov
@@ -427,6 +429,7 @@ | |||
metadataSidePanelContent: null, | |||
mobileSidePanelIsOpen: false, | |||
usingMeteredConnection: true, | |||
isNetworkLibraryAvailable: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we could address part of @LianaHarris360's concern by defaulting this to true
? Then once we've tried to load the network devices, the event you're emitting will then set this to false?
deep: true, | ||
}, | ||
}, | ||
mounted() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect using mounted here might be a little flaky, as we don't actually know that the initial data has been loaded at the point of the component being mounted?
Maybe we can use the isLoadingChannels
value from the useDevices
composable to do an initial trigger here rather than mounted? So the first time that goes from true
to false
we can then trigger this method.
This is much closer - one other slight issue, when there are resources available on my device, I am seeing this. I think a little bit more cleanup to take care of this, and the "not quite loaded" state that @LianaHarris360 flagged and we should be good to go. |
The initially requested changes have been addressed.
@radinamatic Tagging for a QA re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues I was seeing previously appear to be resolved - will leave for QA to give the final thumbs up here!
I'll probably have to revisit this on Monday as it is getting late here but so far the only issues I am observing are on slow internet connection where if there are other libraries and I refresh the page I am seeing first the search panel, then "There's nothing in your library yet..." and "No other libraries around you right now", then "No resources available" and after that the correct state: slow.connection.mp4Similarly if there are no libraries: slow.connection.-.no.libraries.mp4 |
I suspect that these loading state problems have probably been better surfaced by @AllanOXDi's work here, rather than created by it. We'll wait for the rest of @pcenov's review on Monday, but unless there are other significant issues we've not yet encountered, I'd rather put the loading state concerns into a follow up issue rather than block this PR any further. |
@rtibbles I have filled a follow-up for the loading state problems. |
Summary
Fixes the showing of "No resources available" instead of the available other libraries.
Before
After
Library0.17.5.mp4
closes #13071
References
#13071