Is it a bad practice to have a eventListener inside the function of another eventListener?

Question

My code is running as expected, I am creating a dropdown menu from an array. I am then taking the value of each dropdown and assigning a variable to a number based on the selection of the dropdown. Then displaying the variable results in a p> by innerHTML. (I tried doing appendChild(document.createTextNode) instead of innerHTML but it would just keep adding the results to the p>.

I need it to change (not add) based off the dropdown). Furthermore, I need the variable result (the Number) and the event.target.value information of the dropdown to be passed down to another eventlistener on an input field that takes the users input and multiplies that input from the variable in the first eventlistener. This then inject the innerHTML on a second <p>.

I got this working but the question is is this bad practice to put an eventlistener inside another eventlistener? Is there another solution? I tried pulling out the first callback function and creating its own function with the variable being returned. I called it to the second eventlistener but items ended up being undefined (event.target.value in particular).

Here is my code base:

HTML

<select id='cameraMakes' class='cameraSelects'></select>
     <p id='yourCrop'></p>
        <input id="length" type="text" name="lens" placeholder="Enter lens mm a" /><br>
        <p id='results'></p>

JS

const cameraMakeArray = ['Canon5DM2', 'PanasonicGH5', 'SonyA7CropMode']
const cameraMake = document.getElementById("cameraMakes")
const length = document.getElementById("length") 
const yourCrop = document.querySelector("#yourCrop")
const results = document.querySelector("#results")

cameraMakeArray.forEach(camera => {
    let opt = document.createElement('option');
    opt.innerHTML = camera;
    opt.value = camera;
    document.createElement
    cameraMake.appendChild(opt);
    })

cameraMake.addEventListener('change', (event) => {
    let crop = 0
    if (event) {
        results.innerHTML = '';
        length.value = ''
    }
    if (event.target.value === 'Canon5DM2') {
        crop = 1;
    } else if (event.target.value === 'PanasonicGH5') {
        crop = 2;
    } else if (event.target.value === 'SonyA7CropMode') {
        crop = 1.5
    }
    yourCrop.innerHTML = `The ${event.target.value} has a ${crop}x factor`;
    length.addEventListener('input', () => {
        if (length.value) {
        results.innerHTML = `A ${length.value}mm lens is equivalent to a ${length.value * crop}mm lens on the ${event.target.value}`
        } else {
            results.innerHTML = ''
        }
    })
})

Answer 1

It depends on each use case. But, in your case, it's excessive because you don't really want a completely new function each time the select is changed, you only want to change the output. If you just widen the scope of crop, you can set the input handler just once.

Also, you should not use .innerHTML when the string involved doesn't contain any HTML because .innerHTML has performance and security implications. Use .textContent instead.

const cameraMakeArray = ['Canon5DM2', 'PanasonicGH5', 'SonyA7CropMode']
const cameraMake = document.getElementById("cameraMakes")
const length = document.getElementById("length") 
const yourCrop = document.querySelector("#yourCrop")
const results = document.querySelector("#results")

// With the output variable stored at a higher scope than
// either callback function, one function can set it and the
// other can use it. This allows you to get rid of the nested
// event handler.
let crop = 0

cameraMakeArray.forEach(camera => {
    let opt = document.createElement('option');
    opt.textContent = camera;
    opt.value = camera;
    document.createElement
    cameraMake.appendChild(opt);
});

cameraMake.addEventListener('change', (event) => {
    if (event) {
        results.textContent = '';
        length.value = ''
    }
    if (event.target.value === 'Canon5DM2') {
        crop = 1;
    } else if (event.target.value === 'PanasonicGH5') {
        crop = 2;
    } else if (event.target.value === 'SonyA7CropMode') {
        crop = 1.5
    }
    yourCrop.textContent = `The ${event.target.value} has a ${crop}x factor`;
});

length.addEventListener('input', (evt) => {
        if (length.value) {
        results.textContent = `A ${length.value}mm lens is equivalent to a ${length.value * crop}mm lens on the ${cameraMake.options[cameraMake.selectedIndex].textContent}`
        } else {
            results.textContent = '';
        }
});

<select id='cameraMakes' class='cameraSelects'></select>
<p id='yourCrop'></p>
<input id="length" type="text" name="lens" placeholder="Enter lens mm a" /><br>
<p id='results'></p>

You may also want to consider changing the select array values to an object. That way you can store keys along with the values and you wouldn't have to do any if/then to set the variables based on the selection. Also, if you separate your second callback into a named function, you can call it when the select changes to get an immediate update in the output area.

// Now, each camera can store a key along with a value:
const cameras = {
  Canon5DM2: 1,
  PanasonicGH5: 2,
  SonyA7CropMode: 1.5
};

const cameraMakes = document.getElementById("cameraMakes")
const length = document.getElementById("length") 
const yourCrop = document.querySelector("#yourCrop")
const results = document.querySelector("#results")

// Loop through the object:
for(camera in cameras){
    let opt = document.createElement('option');
    opt.textContent = camera;
    opt.value = cameras[camera]; // Get the value that goes with the key
    cameraMakes.appendChild(opt);
}

cameraMakes.addEventListener('change', (event) => {
  yourCrop.textContent = 
    `The ${cameraMakes.options[cameraMakes.selectedIndex].textContent} has a ${cameraMakes.value}x factor`;
 if(results.textContent !== ""){
   displayResults(); // Update the output because the camera changed
 }
});

// By making this a function declaration, you can call it manually
function displayResults() {
  results.textContent = 
    `A ${length.value}mm lens is equivalent to a ${length.value * cameraMakes.value}mm lens on the ${cameraMakes.options[cameraMakes.selectedIndex].textContent}`;
}

length.addEventListener('input', displayResults);

<select id='cameraMakes' class='cameraSelects'></select>
<p id='yourCrop'></p>
<input id="length" type="text" name="lens" placeholder="Enter lens mm a" /><br>
<p id='results'></p>

Answer 2

It's not bad practice, but it's incorrect here

In your use case, you are creating a new event listener every time a change is applied to #cameraMake, therefore, after five changes there will be five event listeners for the input event on #length.

What you want to do is create the event listener once, so removing it from the change event listener is the correct use case.

// create crop here so you can access it in either listener
let crop = 0;

cameraMake.addEventListener('change', event => {
  // modify crop
  crop = /* a value */
})

// attached only once
length.addEventListener('input', () => {
  // instead of using `event.target.value` use `cameraMake.value`
  // you can now access crop here
})

When to nest event listeners

A correct use case for placing an event listener inside of another one could be creating an event listener that only fires once after the outer listener fires.

Note: there are other use cases, be sure to examine how your code works when nesting event listeners, and remove extra/stale event listeners.

Example:

function listener(event) {
  // defined here so it can be removed
}
el1.addEventListener('some-event', event => {
  el2.removeEventListener('some-event', listener)
  el2.addEventListener('some-event', listener, { once: true })
})

Answer 3

Have a look at this version

const cameraMakeObject = {
  'Canon5DM2': 1,
  'PanasonicGH5': 2,
  'SonyA7CropMode': 1.5
}
const cameraMake = document.getElementById("cameraMakes")
const length = document.getElementById("length")
const yourCrop = document.querySelector("#yourCrop")
const results = document.querySelector("#results")
let crop = 0

const calc = function() {
  results.textContent = (length.value) ? 
     `A ${length.value}mm lens is equivalent to a ${length.value * cameraMakeObject[cameraMake.value]}mm lens on the ${cameraMake.value}` : '';
};

Object.keys(cameraMakeObject).forEach(camera => {
  let opt = document.createElement('option');
  opt.innerHTML = camera;
  opt.value = camera;
  cameraMake.appendChild(opt);
})

cameraMake.addEventListener('change', function() {
  let val = cameraMakeObject[this.value] || "unknown"
  yourCrop.innerHTML = val === "unknown" ? "" : `The ${this.value} has a ${val}x factor`;
})

length.addEventListener('input', calc)

<select id='cameraMakes' class='cameraSelects'>
<option value="">Please select</option>
</select>
<p id='yourCrop'></p>
<input id="length" type="text" name="lens" placeholder="Enter lens mm a" /><br>
<p id='results'></p>