Nice, this looks like it does the job and is easy enough to follow along! I believe it’s possible to simplify things a little further, even. There are a few ways to approach it, but rather than searching for the index of the target each time, you could simply decorate each target with HTML (non-Stimulus) data that declares the index. That means that each form element looks something like:
<input data-target="inventory.size" data-index="1">24</input>
The resulting code (with some other changes I’ll walk through) looks like this:
import { Controller } from "stimulus"
export default class extends Controller {
static targets = [ 'bottles',"wbottles", "cbottles","cases",'total','size','percent','ckd']
updateBeer(evt) {
let index = evt.target.dataset.index
this.updateBeerRowTotal(index)
this.checkRow(index)
}
updateLiquor(evt) {
let index = evt.target.dataset.index
this.updateLiquorRowTotal(index)
this.checkRow(index)
}
updateBeerRowTotal(index) {
this.totalTargets[index].value = this.wbottles(index) + this.cbottles(index) + (this.cases(index) * this.size(index) )
}
updateLiquorRowTotal(index) {
const shots = (this.size(index) * (this.percent(index) / 100.0)) / 35.5
const bottles = (this.bottles(index) * this.size(index)) / 35.5
this.totalTargets[index].value = Math.round(bottles + shots)
}
checkRow(index) {
this.ckdTargets[index].checked = true
}
cases(index){
return value('cases', index)
}
wbottles(index){
return value('wbottles', index)
}
cbottles(index){
return value('cbottles', index)
}
size(index){
return value('size', index)
}
bottles(index){
return value('bottles', index)
}
percent(index){
return value('percent', index)
}
value(type, index) {
return Number( this[`${type}Targets`][index].value )
}
}
The total lines of code is about the same, but the event handlers no longer have to look up a target each time, they just grab the index data from the element that changed and pass that along. And there’s no need to keep an instance variable around that’s only meant to be used during the lifecycle of an event.
Now, if you wanted to take things a step further (for instance if you wanted to add a bunch of functionality to the app and thus things were about to get more complex), you could go the route of, for instance, having BeerRow
and LiquorRow
classes (not components) that get instantiated on initialize
and encapsulate their behavior. Your two update events could become one and look like:
updateRow(evt) {
this.rows[evt.target.dataset.index].update()
}
Now, bringing this all full circle - while the single controller approach is certainly better for memory and initialization time, I just checked Basecamp’s own site and they’re not shy about adding dozens of Stimulus components on a page, and it performs great. Thus, without doing any testing to check memory usage, I’m inclined to say that unless you’re running this on very low powered hardware you’ll have simpler, easier to maintain code by just doing one component per row
One last thing - there’s no need (and it’s strongly recommended against) to use eval
. Rather than:
const liquorTargets = eval(`this.${liquorTarget}Targets`)
you can (and should) do:
const liquorTargets = this[`${liquorTarget}Targets`]
Hope this was useful!