✨ ENHANCE: Add Feat/busy indicator

[stevejpurves]

Added a busy indicator to cells which addresses issue #422

This is achieved by adding a css spinner, a new element, and some jquery around the requestExecute promise.

This also works when restart and run all fires.

Reflecting busy status could be taken further as discussed in the issue, but probably in other ways too.

[choldgraf]

This looks great, thanks for the improvement :-)

LGTM with a few thoughts:

• Will the spinner eat up a lot of CPU for users? I don't think it's a huge deal, but I've seen CSS spinning turn on laptop fans before :-)
• You deleted package-lock...that intentional?
• Can you fix the lint test?

As an aside, it would be great if we could get #298 in so that we can test these PRs out more easily

[moorepants]

We've had requests for this with LibreTexts users. A welcome addition!

[moorepants]

This looks good to merge from me, other than the deleted json file and the test. It seems the failure has something to do with the deleted file.

[stevejpurves]

Re: the package-lock.json

It was an intentional update but after looking deeper into the test failure I'd like to back out of that for now. So I am intending to revert.

However, I think there is a problem with the git workflow itself. In the lint step, as well as running npm run fmt we also do a git diff on the local checked-out repo.

That is what is failing as, in that step we also do an npm install. npm install will install fresh packages and create a new package-lock.json file, this newly updated file is different from the checked-in version because we have loosely versioned packages in our package.json. i.e. we are using the ^ on package versions. This is fine as long as we use npm ci instead of npm install in the github action.

I've changed the workflow to use npm ci

We can update the package-lock.json further when we bring in dependabot alerts.

[moorepants]

Yes, this git-diff thing has caused me headaches in the past too. I didn't understand why it is there.

[moorepants]

Here is the issue I discussed the diff: #382

[stevejpurves]

OK, this is still failing for me because I am running with npm v6.x locally. It seems like npm v7.x is now the minimum supported version (looking at @moorepants's comments here #383)

We need to update engines in package.json to prevent this problem. Currently we have:

  "engines": {
    "node": ">=6",
    "npm": ">=4"
  },

npm can go to >=7, but should we also move node to >=15? v15 was the first to ship npm v7 by default.

[stevejpurves]

@choldgraf I think these spinners are ok. They are a single div with a single css transform, so the most minimal possible. tested locally and left them running and didn't see any blip in cpu. Not exactly a performance test but they seem ok.

[moorepants]

I'm fine with the lower node versions if you can get things to work. My "fix" with npm 15 was because that's all I could figure out. There did seem to be a mismatch in what node/npm versions were install locall vs the CI, but part of that is me not really understanding all this in javascript dev.

[stevejpurves]

ok, it's all green now.

So we now have the engines and min requirements in the docs pointing to the same version as in use in CI. These are also the most recent versions so that is a good thing imo.

Note this won't force users to upgrade their node environments, they can still use earlier ones but get warning messages during install but it does mean there is a clear minimum requirement that developers should meet.

[choldgraf]

This looks good to me - thanks for the improvement @stevejpurves. I'll let somebody else with more JS knowledge than myself push the green button, or I'll press it in a day or two if nobody else chimes in :-)

[choldgraf]

thanks @stevejpurves for the improvement!

[moorepants]

The lint test failed on master. We need to dig into that a bit more.