[#10788] Instructor view student list: incorrect state shown very briefly (feedback session page)

[jianhandev]

Fixes #10788

Outline of solution:

• Add a boolean flag hasCourseLoaded, set to true once the courses are successfully fetched from the server. Originally, there were only two flags isCoursesLoading and hasCourseLoadingFailed.
• Use the hasCourseLoaded flag in conjunction with the length of the courseCandidates array to decide whether an error message should be displayed.

Before (notice the red outline around the select drop-down that appears for half a second):

After:

[rrtheonlyone]

I am wondering what is the need for the extra flag here.

Why not just render the component only when this.isCoursesLoading = false?

[jianhandev]

Why not just render the component only when this.isCoursesLoading = false?

You have a point. Originally I assumed if courses aren't loading, it can either be that (i) the courses are already successfully loaded or (ii) the courses failed to load.

However, looking back at the code I realise the only time isCoursesLoading is set to false is when the courses have been successfully loaded.

Conclusion: isCoursesLoading can be used instead of an additional flag.

I have updated the PR to reflect the following changes:

• hasCourseLoaded flag is removed from the InstructorSessionsPageComponent
• in instructor-sessions-page.component.html: set [hasCourseLoaded]="!isCoursesLoading"

[t-cheepeng]

LGTM

In the future, if the bug/change happens very quickly, you can throttle your speed in developer console or add a pipe/delay to the component manually

[rrtheonlyone]

Nice work 👍

However, I believe we can improve this further.

We know that an instructor needs courses to be loaded before he can create a session. The current solution allows for the instructor to still click on Add Session where he will be waiting before he can create anything. This behaviour will still be puzzling as he will find himself not being able to do anything (even if there is no explicit error).

Instead of the current proposal, why not disable the add button till sessions are loaded? This makes more sense from an end-user perspective as the user will see a loading icon and will realise he has to wait first. It also does not add an extra variable as @Input() to our session edit form component.

[jianhandev]

why not disable the add button till sessions are loaded

Hi @rrtheonlyone do you mean we disable the 'Add New Feedback Session' button until courses are created/exists (i.e. at least one course available)?

[rrtheonlyone]

we disable the 'Add New Feedback Session' button until courses are created/exists (i.e. at least one course available)?

No I was thinking disable till the courses have been loaded. Do you think that will work?

[jianhandev]

No I was thinking disable till the courses have been loaded. Do you think that will work?

I see, I think it will work, let me have a go at it.

[jianhandev]

No I was thinking disable till the courses have been loaded. Do you think that will work?

Sorry I realise this doesn't work as the problem still exists if the user does the following:

1. Load the session edit form by clicking on the 'Add New Feedback Session' button
2. Refresh the page

I will like to propose a different approach, which is to load the session edit form only when isCoursesLoading is false (i.e courses has loaded).

Advantages of this approach over using a boolean flag:

• it requires minimum changes to the existing code
• does not add an extra variable as @input() to the session edit form component

[rrtheonlyone]

Your approach works and I am fine with it. Although I still feel disabling till it loads is cleaner and will be more natural for a user (he/she will know that the button cannot be clicked till the loading icon is finished).

With regards to the refreshing, I tried it myself and it seems to work. When the page refreshes, does it not just refetch the courses again? (so the button goes back to being disabled till its fetched)

[jianhandev]

I still feel disabling till it loads is cleaner and will be more natural for a user (he/she will know that the button cannot be clicked till the loading icon is finished).

Yep I agree with this part, although it lasts for only a fraction of a second on my computer but still I guess loading time varies between devices and should be taken into account.

With regards to the refreshing, I tried it myself and it seems to work. When the page refreshes, does it not just refetch the courses again? (so the button goes back to being disabled till its fetched)

Yes the button will go back to being disabled once refreshed until courses are fetched. Assuming that the session edit form is in the collapsed state, it will be perfectly fine. However, there's another situation where the session edit form might be expanded already when the user refreshes, that's why I proposed to only expand the session edit form when the courses are loaded.

I have updated the PR to disable the Add New Session button when courses are still loading.

[rrtheonlyone]

However, there's another situation where the session edit form might be expanded already when the user refreshes, that's why I proposed to only expand the session edit form when the courses are loaded.

Steps to reproduce the above? Even if it is expanded, I believe it goes back to being closed on page refresh.

[jianhandev]

Even if it is expanded, I believe it goes back to being closed on page refresh.

This is not always the case. To reproduce, you can follow the steps in the gif below:

[rrtheonlyone]

Ah yes your right.. I wonder: what about going with a combination of both? You disable the button and not load the session edit form while courses are being fetched from API. This way we can resolve this issue.

[jianhandev]

I wonder: what about going with a combination of both?

Yes! The current solution incorporates both, the button is disabled when the courses are loading and the edit section will expand once the courses are loaded.

[rrtheonlyone]

LGTM 💯 Great work!