removed isLoading #804
removed isLoading #804
Conversation
wxtim
force-pushed
the
removed.isLoading
branch
from
54f337c to
e70321c
Compare
|
@kinow - I don't exactly what hours you keep relative to your time zone, but I wouldn't mind a quick chat if this isn't it. It doesn't seem like much. I head for bed about 10. |
There was a problem hiding this comment.
Hi @wxtim ! I started working a bit later this morning 😪 sorry. See my review comments, I think you are nearly there in this PR. I've pointed what needs to be removed, if you could amend this PR once more (sorry!), then it should be ready for review+merge.
Since you are modifying the Vuex store here too, you may find interesting reading just the first paragraphs of the Vuex docs for mutations and for actions (you can ignore the rest, but just so the distinction of action & mutation in Vuex is clear 😬 )
| /** | ||
| * Whether the application is loading or not. | ||
| */ | ||
| isLoading: false, | ||
| /** |
There was a problem hiding this comment.
Ah, you had it right before in this file, sorry the confusion. There is an action1 (setLoading({ commit} ...)), and a mutation2 (SET_LOADING(state, ...)3) in this file that can be deleted too.
Footnotes
-
an action is asynchronous ↩
-
a mutation is synchronous ↩
-
we started implementing both always, but in some cases we don't need an async action, it's useful when you have a request that takes a long time and you don't want to block the UI; also actions are camel-case, and mutations upper-case by convention ↩
There was a problem hiding this comment.
Then you also need to delete the tests for the store state isLoading, see tests/unit/store.index.spec.js. They are grouped under the describe('isLoading'). They can be deleted.
And in src/plugins/axios.js, remove store.dispatch('setLoading', ...)1 too.
After that the PR should be ready to be merged 🎉
Footnotes
-
you dispatch a Vuex action (async), and commit a Vuex mutation (sync, blocking)
After that this PR should be ready to be merged 🎉 ↩
There was a problem hiding this comment.
This is what I thought I needed to do, but the UI didn't work for me after I did it. 😞
There was a problem hiding this comment.
Oh, interesting. Were there any errors in the browser console, or in the terminal where you were running yarn run serve or yarn run build:watch?
There was a problem hiding this comment.
After quickly peeking at the last commit, @wxtim , I think this one is missing?
And in src/plugins/axios.js, remove store.dispatch('setLoading', ...) lines too.
There was a problem hiding this comment.
Massive thanks for holding my hand through that one. Main learning point - just bc it's a new language don't forget all the lessons you've learned in Python about reading traceback carefully.
@kinow Do you have an opinion on the Google Javascript style guide? @matthewrmshin suggested the Google Bash style guide as useful reading when I asked for a Bash opposite number to PEP8 - would that work here?
Is the code in axios.js actually doing anything now? It looks to me like it's intercepting requests and responses and doing nowt. Am I right? Is this now just form code that the Axios plugin requires? Does Axios do stuff by default?
There was a problem hiding this comment.
Massive thanks for holding my hand through that one. Main learning point - just bc it's a new language don't forget all the lessons you've learned in Python about reading traceback carefully.
Don't mention it. Thanks for volunteering to work on this issue!
@kinow Do you have an opinion on the Google Javascript style guide? @matthewrmshin suggested the Google Bash style guide as useful reading when I asked for a Bash opposite number to PEP8 - would that work here?
I tried a few different styles for JavaScript, but what I looked were eslint shareable configurations (or style guides). I remember reading the Airbnb rules, but never checked the Google rules (I think they must be following the Google Javascript style you mentioned.)
We extend three rules, and apply a few more here. If there are things you think would improve readability/maintainability, we would probably start by customizing these rules or adopting other style guides, so our CI can verify them (IDE's/editors normally integrate with these style guides too.)
eslint is executed in our CI, see package.json for the yarn run lint script.
Is the code in axios.js actually doing anything now? It looks to me like it's intercepting requests and responses and doing nowt. Am I right?
You are right! Good catch! It was intercepting HTTP requests done with Axios (e.g. the HTTP request to Jupyter{Hub|Server} to fetch the user data). Looks like we can actually ditch the axios plug-in. Feel free to update this PR for that, or if you prefer we can start a separate PR or just create an issue. Up to you :-)
There was a problem hiding this comment.
Looks like we can actually ditch the axios plug-in.
Done
Codecov Report
@@ Coverage Diff @@
## master #804 +/- ##
==========================================
+ Coverage 90.96% 91.10% +0.13%
==========================================
Files 84 86 +2
Lines 1672 1686 +14
Branches 105 105
==========================================
+ Hits 1521 1536 +15
+ Misses 121 120 -1
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
These changes close #783
Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.