NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
28 Apr 2019

This is harder to test but is cleaning up some tech debt in the code base. In some places we were directly manipulating the vuex state, which as documented in the code shouldn't be done. I've cleaned that up to correctly use a mutator. To test ensure that the rename modal works before and after the patch in media manager

In addition if you now build media manager in dev mode (or watch which is dev mode by default) we now enable vuex strict mode (note as per the docs you shouldn't use strict mode in production https://vuex.vuejs.org/guide/strict.html). This means in the future when people do dev work they should get warnings when they do such bad things.

@dneukirchen are you able to do a code review here please

avatar wilsonge wilsonge - open - 28 Apr 2019
avatar wilsonge wilsonge - change - 28 Apr 2019
Status New Pending
avatar wilsonge wilsonge - change - 28 Apr 2019
Title
Fix various instances where we were doing bad things in mvuex
[4.0] Fix various instances where we were doing bad things in vuex
avatar wilsonge wilsonge - edited - 28 Apr 2019
avatar joomla-cms-bot joomla-cms-bot - change - 28 Apr 2019
Category Administration com_media NPM Change JavaScript
avatar wilsonge wilsonge - change - 28 Apr 2019
The description was changed
avatar wilsonge wilsonge - edited - 28 Apr 2019
avatar wilsonge wilsonge - change - 28 Apr 2019
The description was changed
avatar wilsonge wilsonge - edited - 28 Apr 2019
avatar wilsonge
wilsonge - comment - 29 Apr 2019

Why is this needed? 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV)

I took it from the docs on the environment plugin here. It gave it's equivalent using the define plugin https://webpack.js.org/plugins/environment-plugin/ (and it seemed to work for me). I always have to google this one every time...

Imo we dont need 2 way binding for the renaming. It might seem nice in the first place, but iirc the inline change is hidden behind a modal backdrop, so we can simplify things with 1 way databinding aka:

We can but then rather than do anything on change. We should just wait for the user to hit save. And use the existing save function. And remove the whole responsiveness other than for giving feedback on validity.

personal opinion: i do not like words like "do", "run", "execute", "perform" or "process" xxx in method or var names, so i would rename "perform_rename" to "rename" or "change_name"

I have no preference on naming conventions. Done CHANGE_NAME (although obviously this will go entirely if we do the above which seems logical)

FYI: we had discussions about removing vuex from mm to avoid the additional complexity it brings to the table. What’s your opinion on that?

I don't think you'll save yourself much complexity because for things like showing the modals we'll have to pass deep nested functions through the application (also for example dealing with selected items interaction with the toggle all). And that will become a maintenance issue as well (especially as it's "bad practice" for any of these js apps).

avatar wilsonge wilsonge - change - 29 Apr 2019
Labels Added: NPM Resource Changed ?
avatar wilsonge
wilsonge - comment - 29 Apr 2019

OK Second commit removes everything and just does the update in the existing RENAME_SUCCESS mutation. Which means it's only updating the UI after the API gives a success which is also much better from a sanity perspective. Before now if the API response failed we'd updated the UI like it was a success.

avatar wilsonge wilsonge - change - 1 May 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-05-01 21:33:07
Closed_By wilsonge
avatar wilsonge wilsonge - close - 1 May 2019
avatar wilsonge wilsonge - merge - 1 May 2019

Add a Comment

Login with GitHub to post a comment