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

Fixes loading states in general #10455

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
224e863
lessonplaylistpage << kcircularloader when loading
nucleogenesis Apr 4, 2023
919f277
use core vuex loading state in LearnAppBarPage rather than taking prop
nucleogenesis Apr 11, 2023
90ae11d
mock newly expected promise in Bookmark spec
nucleogenesis Apr 11, 2023
7d80f00
kcircularloader -> userstable during loading state
nucleogenesis May 10, 2023
bde78d3
KCircularLoader -> TopicsPage
nucleogenesis May 17, 2023
8eea333
lessonplaylistpage << kcircularloader when loading
nucleogenesis Apr 4, 2023
97f6a99
use core vuex loading state in LearnAppBarPage rather than taking prop
nucleogenesis Apr 11, 2023
d500a97
ManageChannelsPage KCircularLoader while loading channels
nucleogenesis May 17, 2023
c47fbac
CoreTable accepts dataLoading prop; show loader when loading
nucleogenesis May 17, 2023
dd4c4dd
manage permissions loading state, drill to CoreTable
nucleogenesis May 17, 2023
773edc4
usertable accepts and passes a dataLoading prop to coretable
nucleogenesis May 18, 2023
db9602b
facility class*Management loading state in local vuex, remove unused …
nucleogenesis May 18, 2023
d942bda
conditionalize emptyMessage properly in CoreTable
nucleogenesis May 18, 2023
6d0e51c
class edit page tables pass dataloading
nucleogenesis May 18, 2023
fb76682
remove unnedeeded core.loading check in usertable
nucleogenesis May 18, 2023
b4a797a
facility userpage manages dataloading state for table
nucleogenesis May 18, 2023
aaa1c84
manageclasspage classes list uses dataLoading
nucleogenesis May 18, 2023
ed70564
coach:86 busy, add dataLoading state; implement in *ClassListPage's c…
nucleogenesis May 18, 2023
f5153e9
reset coach class list when calling action to get new list
nucleogenesis May 30, 2023
30dd238
remove unused style in usertable
nucleogenesis May 30, 2023
678da96
just assign to the state variable directly for loadingFacilityUsers
nucleogenesis May 30, 2023
70179a8
Core/AppBarPage prop.loading default from core vuex; drill LearnABP.l…
nucleogenesis May 30, 2023
ce235f3
remove samePageCheck business from classEditManagement handler
nucleogenesis May 30, 2023
caa6449
nix another samePageCheck in facility handlers; use more common promi…
nucleogenesis May 30, 2023
97e3d28
CoreTable use logging.error
nucleogenesis May 30, 2023
818d765
remove all uses of finally() outside of JS spec files
nucleogenesis Jun 2, 2023
c15cbe6
make querying the loading state default to false to appease tests
nucleogenesis Jun 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion kolibri/core/assets/src/views/CorePage/AppBarPage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@

import { mapGetters } from 'vuex';
import { get } from '@vueuse/core';
import _get from 'lodash/get';
import LanguageSwitcherModal from 'kolibri.coreVue.components.LanguageSwitcherModal';
import ScrollingHeader from 'kolibri.coreVue.components.ScrollingHeader';
import useKResponsiveWindow from 'kolibri.coreVue.composables.useKResponsiveWindow';
Expand Down Expand Up @@ -100,7 +101,9 @@
},
loading: {
type: Boolean,
default: null,
default() {
return _get(this, '$store.state.core.loading', false);
},
},
},
data() {
Expand Down
28 changes: 24 additions & 4 deletions kolibri/core/assets/src/views/CoreTable.vue
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
<script>

import get from 'lodash/get';
import logger from 'kolibri.lib.logging';
import KCircularLoader from 'kolibri-design-system/lib/loaders/KCircularLoader';

const logging = logger.getLogger(__filename);

export default {
name: 'CoreTable',
components: { KCircularLoader },
props: {
selectable: {
type: Boolean,
Expand All @@ -14,6 +19,11 @@
type: String,
default: null,
},
dataLoading: {
type: Boolean,
default: false,
required: false,
},
},
computed: {
tHeadStyle() {
Expand Down Expand Up @@ -73,17 +83,27 @@
}
});

// Insert an empty message as a <p> at the end if it is provided and the
// table has no rows.
const showEmptyMessage = this.emptyMessage && !tableHasRows;
// If we have loaded the data, but have no empty message and no rows, we log an error.
if (!this.dataLoading && this.emptyMessage && !tableHasRows) {
logging.error('CoreTable: No rows in table, but no empty message provided.');
}

/*
* If data is still loading, then we show a loader. Otherwise, we show the
* empty message if there are no rows in the table. If we have loaded data, have
* an emptyMessage and have no rows. If we have rows, then we show the table alone
*/
var dataStatusEl = this.dataLoading
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking, just curious!

is there a reason this is a var vs. a const? it appears like its value doesn't get reassigned (just conditionally assigned) and it doesn't need to be accessed beyond this scope, but i think i'm missing something & i'd love to learn what it is! 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed it to be a var in an earlier failed attempt to write this bit and I forgot to change it to const after getting it settled

Copy link
Contributor

Choose a reason for hiding this comment

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

i just assumed it was for some technical reason that went over my head! 😅

? createElement('p', [createElement(KCircularLoader)])
: !tableHasRows && createElement('p', this.emptyMessage); // Only show message if no rows

return createElement('div', { class: 'core-table-container' }, [
createElement('table', { class: 'core-table' }, [
...(this.$slots.default || []),
theadEl,
tbodyCopy,
]),
showEmptyMessage && createElement('p', this.emptyMessage),
dataStatusEl,
]);
},
};
Expand Down
5 changes: 3 additions & 2 deletions kolibri/core/assets/src/views/ExamReport/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -437,13 +437,14 @@
MasteryLogResource.fetchMostRecentDiff(this.getParams())
.then(currentTry => {
this.currentTry = currentTry;
this.loading = false;
})
.catch(err => {
if (err.response && err.response.status_code === 404) {
this.$emit('noCompleteTries');
}
})
.finally(() => (this.loading = false));
this.loading = false;
});
},
loadAllTries() {
MasteryLogResource.fetchCollection({ getParams: this.getParams(), force: true }).then(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { shallowMount, mount } from '@vue/test-utils';
import { mount } from '@vue/test-utils';
import { UserKinds } from 'kolibri.coreVue.vuex.constants';
import { coreStoreFactory } from 'kolibri.coreVue.vuex.store';
import UserTable from '../index';

function makeWrapper({ propsData } = {}) {
const store = coreStoreFactory({});
store.dispatch('notLoading');
return mount(UserTable, {
store,
propsData,
});
}
Expand Down Expand Up @@ -36,7 +40,7 @@ const TEST_USERS = [

describe(`UserTable`, () => {
it(`smoke test`, () => {
const wrapper = shallowMount(UserTable);
const wrapper = makeWrapper();
expect(wrapper.exists()).toBeTruthy();
});

Expand Down
13 changes: 5 additions & 8 deletions kolibri/core/assets/src/views/UserTable/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
data-test="selectAllCheckbox"
@change="selectAll($event)"
/>
<CoreTable>
<CoreTable :emptyMessage="emptyMessage" :dataLoading="dataLoading">
<template #headers>
<th data-test="fullNameHeader" :style="{ minWidth: '32px' }">
<span v-if="selectable" class="visuallyhidden">
Expand Down Expand Up @@ -188,13 +188,6 @@
</template>
</CoreTable>

<p
v-if="!users || !users.length"
class="empty-message"
>
{{ emptyMessage }}
</p>

</div>

</template>
Expand Down Expand Up @@ -270,6 +263,10 @@
type: String,
default: '',
},
dataLoading: {
type: Boolean,
default: false,
},
},
computed: {
showSelectAllCheckbox() {
Expand Down
13 changes: 11 additions & 2 deletions kolibri/plugins/coach/assets/src/modules/pluginModule.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const logging = logger.getLogger(__filename);
export default {
Copy link
Contributor

@thanksameeelian thanksameeelian May 25, 2023

Choose a reason for hiding this comment

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

non-blocking but potentially important to address in a further loading state or coach-specific PR if not here:

i just wanted to re-raise the issue here as it is not caused or worsened by your work but is slightly affected by it: the oddness in loading in Coach as a multi-facility user (outlined in #10714 & detailed further in this comment) was also noticeable during review of your PR, because now when a multi-facility user goes to look at the classes in one of their imported facilities and the default facility's class list is briefly displayed before the imported facility's class list renders and replaces it, the default facility's class list is also decorated with a loading spinner.

coach-multi-facility-classes-page.mov

the scenario definitely was not caused by your PR, but is affected by the addition of the loading spinner. just FYI the same doesn't occur in the somewhat parallel scenario in Facility - it's all looking good there:

facility-multi-facility-as-expected.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... I'll see if maybe we can clear the contents of the page in coach whenever we make a new requests to a different facility or something.

state() {
return {
busy: false,
dataLoading: false,
classList: [],
pageName: '',
toolbarRoute: {},
Expand All @@ -38,6 +38,9 @@ export default {
SET_TOOLBAR_TITLE(state, title) {
state.toolbarTitle = title;
},
SET_DATA_LOADING(state, dataLoading) {
state.dataLoading = dataLoading;
},
SET_TOOLBAR_ROUTE(state, toolbarRoute) {
state.toolbarRoute = toolbarRoute;
},
Expand All @@ -60,13 +63,19 @@ export default {
},
actions: {
setClassList(store, facilityId) {
store.commit('SET_DATA_LOADING', true);
store.commit('SET_CLASS_LIST', []); // Reset the list if we're loading a new one
return ClassroomResource.fetchCollection({
getParams: { parent: facilityId || store.getters.currentFacilityId, role: 'coach' },
})
.then(classrooms => {
store.commit('SET_CLASS_LIST', classrooms);
store.commit('SET_DATA_LOADING', false);
})
.catch(error => store.dispatch('handleApiError', error));
.catch(error => {
store.dispatch('handleApiError', error);
store.commit('SET_DATA_LOADING', false);
});
},
/**
* Handle coach page errors.
Expand Down
9 changes: 3 additions & 6 deletions kolibri/plugins/coach/assets/src/views/CoachClassListPage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,15 @@
<h1>{{ coreString('classesLabel') }}</h1>
<p>{{ $tr('classPageSubheader') }}</p>

<p v-if="classList.length === 0">
<p v-if="classList.length === 0 && !dataLoading">
<KExternalLink
v-if="isAdmin && createClassUrl"
:text="$tr('noClassesDetailsForAdmin')"
:href="createClassUrl"
/>
<span v-else>
{{ emptyStateDetails }}
</span>
</p>

<CoreTable v-else>
<CoreTable v-else :dataLoading="dataLoading" :emptyMessage="emptyStateDetails">
<template #headers>
<th>{{ coreString('classNameLabel') }}</th>
<th>{{ coreString('coachesLabel') }}</th>
Expand Down Expand Up @@ -83,7 +80,7 @@
mixins: [commonCoach, commonCoreStrings],
computed: {
...mapGetters(['isAdmin', 'isClassCoach', 'isFacilityCoach', 'userIsMultiFacilityAdmin']),
...mapState(['classList']),
...mapState(['classList', 'dataLoading']),
// Message that shows up when state.classList is empty
emptyStateDetails() {
if (this.isClassCoach) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,20 @@ function fetchFacilityUsers() {

export function showManagePermissionsPage(store) {
const shouldResolve = samePageCheckGenerator(store);
store.commit('managePermissions/SET_LOADING_FACILITY_USERS', true);
const promises = Promise.all([fetchFacilityUsers(store), fetchDevicePermissions()]);
return promises
.then(function onSuccess([users, permissions]) {
.then(([users, permissions]) => {
if (shouldResolve()) {
store.commit('managePermissions/SET_STATE', {
facilityUsers: users,
permissions,
});
}
return store.commit('managePermissions/SET_LOADING_FACILITY_USERS', false);
})
.catch(function onFailure(error) {
shouldResolve() ? store.dispatch('handleError', error) : null;
.catch(error => {
store.commit('managePermissions/SET_LOADING_FACILITY_USERS', false);
return shouldResolve() ? store.dispatch('handleError', error) : null;
});
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
function defaultState() {
return {
loadingFacilityUsers: false,
facilityUsers: [],
permissions: {},
};
Expand All @@ -15,5 +16,8 @@ export default {
RESET_STATE(state) {
Object.assign(state, defaultState());
},
SET_LOADING_FACILITY_USERS(state, loadingFacilityUsers) {
state.loadingFacilityUsers = loadingFacilityUsers;
},
},
};
24 changes: 14 additions & 10 deletions kolibri/plugins/device/assets/src/views/ManageContentPage/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,23 @@
@clearall="handleClickClearAll"
/>

<p v-if="!channelsAreInstalled">
<p v-if="!channelsAreInstalled && !channelListLoading">
{{ $tr('emptyChannelListMessage') }}
</p>

<div class="channels-list">
<ChannelPanel
v-for="channel in sortedChannels"
:key="channel.id"
:channel="channel"
:disabled="channelIsBeingDeleted(channel.id)"
:showNewLabel="showNewLabel(channel.id)"
@select_delete="deleteChannelId = channel.id"
@select_manage="handleSelectManage(channel.id)"
/>
<KCircularLoader v-if="channelListLoading" />
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking, hoping for additions in a follow-up PR:

having the loading spinner here & in Permissions is really nice, and its helpfulness is highlighted in comparison to the behavior in Info & Facilities within this same plugin, which don't appear to provide any loading indicators. it would be nice to add it in there as well in the future, if possible!

loading-in-content-and-permissions.mov

<div v-else>
<ChannelPanel
v-for="channel in sortedChannels"
:key="channel.id"
:channel="channel"
:disabled="channelIsBeingDeleted(channel.id)"
:showNewLabel="showNewLabel(channel.id)"
@select_delete="deleteChannelId = channel.id"
@select_manage="handleSelectManage(channel.id)"
/>
</div>
</div>

<SelectTransferSourceModal :pageName="pageName" />
Expand Down Expand Up @@ -134,6 +137,7 @@
'managedTasks',
]),
...mapState('manageContent/wizard', ['pageName']),
...mapState('manageContent', ['channelListLoading']),
pageTitle() {
return deviceString('deviceManagementTitle');
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<div>

<CoreTable :emptyMessage="emptyMessage">
<CoreTable :emptyMessage="emptyMessage" :dataLoading="dataLoading">
<template #headers>
<th>{{ coreString('fullNameLabel') }}</th>
<th>{{ coreString('usernameLabel') }}</th>
Expand Down Expand Up @@ -82,6 +82,11 @@
type: Function,
default: () => null,
},
dataLoading: {
type: Boolean,
default: false,
required: false,
},
},
computed: {
...mapGetters(['facilities', 'currentUserId']),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
</div>

<PaginatedListContainer
:dataLoading="loadingFacilityUsers"
:items="usersFilteredByDropdown"
:filterPlaceholder="$tr('searchPlaceholder')"
>
Expand Down Expand Up @@ -44,6 +45,7 @@
</template>
<template #default="{ items, filterInput }">
<UserGrid
:dataLoading="loadingFacilityUsers"
:searchFilter="searchFilterText"
:facilityUsers="items"
:userPermissions="userPermissions"
Expand Down Expand Up @@ -98,6 +100,7 @@
...mapState('managePermissions', {
facilityUsers: state => state.facilityUsers,
userPermissions: state => userid => state.permissions[userid],
loadingFacilityUsers: state => state.loadingFacilityUsers,
}),
...mapState({
query: state => state.query,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import samePageCheckGenerator from 'kolibri.utils.samePageCheckGenerator';
import { ClassroomResource, FacilityUserResource } from 'kolibri.resources';
import { localeCompare } from 'kolibri.utils.i18n';
import { _userState } from '../mappers';
Expand All @@ -17,22 +16,17 @@ export function showClassEditPage(store, classId) {
ClassroomResource.fetchModel({ id: classId, force: true }),
ClassroomResource.fetchCollection({ getParams: { parent: facilityId }, force: true }),
];
const shouldResolve = samePageCheckGenerator(store);
Promise.all(promises).then(
([facilityUsers, classroom, classrooms]) => {
if (shouldResolve()) {
store.commit('classEditManagement/SET_STATE', {
modalShown: false,
currentClass: classroom,
classes: classrooms,
classLearners: sortUsersByFullName(facilityUsers).map(_userState),
classCoaches: sortUsersByFullName(classroom.coaches).map(_userState),
});
store.commit('CORE_SET_PAGE_LOADING', false);
}
},
error => {
shouldResolve() ? store.dispatch('handleError', error) : null;
}
);
store.commit('classEditManagement/SET_DATA_LOADING', true);
Promise.all(promises)
.then(([facilityUsers, classroom, classrooms]) => {
store.commit('classEditManagement/SET_DATA_LOADING', false);
store.commit('classEditManagement/SET_STATE', {
modalShown: false,
currentClass: classroom,
classes: classrooms,
classLearners: sortUsersByFullName(facilityUsers).map(_userState),
classCoaches: sortUsersByFullName(classroom.coaches).map(_userState),
});
})
.catch(error => store.dispatch('handleError', error));
}
Loading