[#746, #1351] Dashboard: show a spinner when the view is changing

[gerhean]

Fixes #1351, #746

Probably fixed, but unsure how to test as need large amount of data for the loading to actually be seen.

Edit:

Before:

Screen frozen and unresponsive.

After:

Example of loading spinner (will ensure link works until PR is merged): https://gerhean.github.io/tp-dashboard/

setTimeout() is used to force this.filtered to update only after browser rerender (loading screen is displayed).

Something to note is that the animated loading indicator uses css to be animated, so it spins even if the website is actually not working. It is basically just eye candy.

Proposed commit message:

For reports with large amount of data, the view can take some time to change.

Looking at a frozen browser tab causes anxiety and might lead one to believe that the tab has stopped working, while in reality it is simply doing background processing.

Let's add an animated loading indicator.

A portion of filterGroupSelection() has been abstracted out for better readability as it had to be wrapped in a setTimeout()

v-bind:key for v-authorship and v-zoom tabs in index.pug has been changed to prevent unnecessary rerendering.

[fzdy1914]

The data can be found here. Since your PR does not edit the file structure. You can just use the data directly. So you may need to clone this tp-dashboard, switch to github page branch, replace the HTML and attached the resultant Github Page link here for us to check your effort.

An even better choice is that, we have a repo for generating reposense using travis or github actions. By modifying the get-reposense.py, you can make it to download the version of your reposense and do the report generation. So you then can automate the process.

The second part can leave as a bonus part, that you submit a PR for our publish RepoSense so it can support --branch tag.

[fzdy1914]

Meanwhile, do remember to follow the development guide in DG. Including the PR title and description.

[gerhean]

Perhaps there should be an article in the DG about how to test frontend without repeatedly generating the data as I am still not entirely sure of the instructions.

[fzdy1914]

Perhaps there should be an article in the DG about how to test frontend without repeatedly generating the data as I am still not entirely sure of the instructions.

This is indeed in our plan. But currently, we have to achieve it using a workaround. You may just terminate the gradlew run process just at the time the console shows that it starts to analyzing the repos. Then, at that timestamp, the html is replaced with new one but the data is still the old one. That is the trick I use when I am a student.

A more standard way is to use gradlew zipReport to generate the html file zip file and manually replace the same file in the destination folder. The zip file is inside the Java src resource folder. Finally, you can use gradlew run -Dargs="-v ./reposense-report" to launch the report to avoid CORS issue.

[fzdy1914]

@damithc Prof, can you help to check the UI? I feel OK, but we may add some text for it, like: Loading Data or Processing Data comparing with the loading resources shown in the first place.

[fzdy1914]

Meanwhile, there is latency time when merge all groups button is clicked.

[damithc]

@damithc Prof, can you help to check the UI? I feel OK, but we may add some text for it, like: Loading Data or Processing Data comparing with the loading resources shown in the first place.

Mostly fine 👍

Shall we use the same spinner used at the very start? No reason to use two different ones.

Yes, we can show something like Working. Please wait ... below the spinner.

[fzdy1914]

@gerhean As pointed out by the Prof, let's use the new loading spinner for both resource loading and data processing. Meanwhile, please add some text below the spinner to notify the user of the different loading stages.

Finally, please look into the loading of v_authorship. If u found it easy, u may add the same loading overlay for the loading stage of v_authorship

[gerhean]

I am having a problem in which v-authorship is being created twice and destroyed once on startup, which causes removeAuthorshipHashes() to be called. This is causing problems in testing but i'm not really sure why it's happening.

[gerhean]

Any suggestions for modifying this key:

I want to change it as it is causing the whole component to rerender multiple times (as the loading screen requires some data to be updated asynchronously). This is causing the beforeDestroy hook to trigger wrongly.

EDIT

I changed the key generation to only use specific object keys, not sure if that would break anything in the future.

[fzdy1914]

@gerhean Good observation of the issue! Actually, there is an attempt to fix it but fails. You may refer to #967 and #1167 for more details. Your proposed implementation is quite nice and can fix it as a workaround. However, I believe the ideal solution is to make use of the re-rendering of Vue directly like what is done in #967 but fix the bug caused by it.

@gerhean Do request a review from me once the PR is finished. Good Work done also, I have recommended you to the Prof.

@jamessspanggg Can you check the implementation also to make sure that the attributes listed are the key components to re-render the sidebar?

[gerhean]

@Tejas2805

I think it might be better to say - Loading. Please wait. - instead of - Working. Please wait.`

Done

Regression bug introduced - Sorting in Code Panel is not working. Sorting is still working in the Commit Panel.

Thanks for telling me. Fixed.

[fzdy1914]

@Tejas2805, your review is needed.

[Tejas2805]

In some cases, the part displayed in the image below shows up first. And then the loading spinner. In some cases, both come together. So if that is not much of an issue, LGTM. @damithc @fzdy1914

reposense.mp4

[fzdy1914]

In some cases, the part displayed in the image below shows up first. And then the loading spinner. In some cases, both come together. So if that is not much of an issue, LGTM. @damithc @fzdy1914

That is indeed what will happen in the current implementation. Changing the implementation will be done in a separate PR.

[damithc]

Memory usage on Edge:

• https://gerhean.github.io/tp-dashboard/ 800-900Mb
• https://nus-cs2103-ay2021s1.github.io/tp-dashboard/ (generated on Dec 4) 600-700Mb

Did we do something since Dec 4 to degrade performance?

[fzdy1914]

Did we do something since Dec 4 to degrade performance?

Don't seem to have done anything that may downgrade the performance.

@gerhean Can you help to check this issue?

[damithc]

Did we do something since Dec 4 to degrade performance?

Don't seem to have done anything that may downgrade the performance.

@gerhean Can you help to check this issue?

I checked again now. Seems the memory usage is similar for both cases. Goes to 1Gb at the start but comes down to 600Mb after some time. So, should be fine. OK to go ahead.

[fzdy1914]

@Tejas2805 Can you review again?

[Tejas2805]

Just reviewed again. I thought we had fixed issue #1081 in PR #1089 but I can see the bug still exists. Has it been re-introduced in this PR?

video.mov

The frontent files should still be hidden and not be shown as being seen in the video.

[fzdy1914]

Just reviewed again. I thought we had fixed issue #1081 in PR #1089 but I can see the bug still exists. Has it been re-introduced in this PR?

That PR is not even merged. So, it is not fixed.

[Tejas2805]

That PR is not even merged. So, it is not fixed.

The issue was closed so thought that it had been fixed. Just checked it was closed by PR #1065

[Tejas2805]

LGTM!