Removing tight coupling in recipe-editing vue components

[seyfeb]

I noticed that the editing components (EditInputField, EditInputGroup, etc.) are tightly coupled with RecipeEdit due to the references from the child to the parent component. As an example, look at the EditInputField component which sets a field in the $parent.

In this PR I removed the tight coupling which should allow/lead to

• a clear one-way dependency of those components
• more flexible ways of data binding
• v-model can be used on those components
• components can be reused in different places.

@sam-19 Could you have a look at this?

[sam-19]

Hey, and sorry for the delay. I'm afraid I don't have quite enough time to review all the Vue-related pull requests at the moment, but one of the great things about Vue is that it's easy to grasp. @seyfeb probably can do it just as well as I at this point.

There is one thing that caught my attention. Some of the components have a prop named 'value' that is not explicitly passed down to the component from the parent. I can only assume this is an implicit feature that binds the v-model attribute to the value prop and as such is probably just fine. My personal design philosophy, however, has been to always avoid implicity for a number of reasons (it relies on features that may change, it may behave unexpectedly in edge scenarios, the code is harder to read or requires intimate familiarity with the framework etc.), and am happy that Nextcloud is (probably) deprecating jQuery in the near future. One potential issue is that if we have to add another input element to one of the components in question, someone has to decide which of them carries the "primary" value and which is only a secondary one. In the end of the day it's still just a matter of taste, but something to think about nevertheless.

Bottom line: I think moving away from tight coupling is the right way to go and if these changes work in a test environment, I'd say go for it. Sorry that I can't devote the time for a proper review right now.

[seyfeb]

tl;dr I’m going to explicitly state which property is used by the v-model binding of the components.

Thank you @sam-19, for your comments!

Regarding v-model some thoughts from my side:

• the two-way data binding is a fundamental feature provided by vue.js (and already discussed on the introduction page, so I’m rather optimistic that it won’t suddenly be gone. As it is introduced to vue beginners very early, I assume that most will understand what it is supposed to do.
• using v-model even makes the code in the parent easier to read since it is less cluttered (no need for specifying v-bind and v-on)
• you have a point with using the implicit value property. In v3 it is even going to be renamed as I just found out. In 2.2, however, the possibility of explicitly defining the property and event name handled by the v-model directive was introduced. Therefore I would propose to update the components to have an explicit statement in the code - making it easier to understand.
• In v3 your problem: "decide which of them carries the "primary" value and which is only a secondary one" will also be solved since you can define multiple models! :)

[codecov]

Codecov Report

Merging #386 (ea56afb) into master (cce3864) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master    #386   +/-   ##
========================================
  Coverage      0.91%   0.91%           
  Complexity      414     414           
========================================
  Files            13      13           
  Lines          1307    1307           
========================================
  Hits             12      12           
  Misses         1295    1295           

Flag	Coverage Δ	Complexity Δ
integration	0.00% <ø> (ø)	0.00 <ø> (ø)
unittests	0.91% <ø> (ø)	0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[christianlupus]

@seyfeb I fear the merging of #402 caused conflicts. Could you please have a look and rebase this branch?

[christianlupus]

I just tested the changes and I found a small bug:

When opening a recipe without any nutrition information attached, I get warnings in the JS console if I change anything (aka trigger events):

[christianlupus]

Another thing I found just now: When creating a new recipe from scratch the nutrition settings are hidden. Saving and reopening for editing allows to set them correctly.

[seyfeb]

Thank you for the feedback. It seems like the rebase didn’t go as smooth as hoped. The problems you found should be fixed now, @christianlupus !