patternjavascriptMinor
Displaying dependent options when a selection is made in a dropdown
Viewed 0 times
selectionmadedropdownoptionsdisplayingwhendependent
Problem
I was wondering if someone could please point me in the right direction with this bit of JavaScript I have been working on.
I have made this form that currently has onblur JavaScript validation and additionally I am to make it dynamic so that when a user selects an option from a dropdown option form a div is shown with corresponding options. But at the moment that code I have written for this is looking pretty repetitive and suffering from 'code smell'.
Is there a better way to do this? I am thinking a for loop that iterates through the dropdown options but am not 100% sure how to put this in place?
```
adventureForm = document.querySelector('#adventure');
charterDrop = document.querySelector('.charter-options');
mountainDrop = document.querySelector('.mountain-lake-options');
trampDrop = document.querySelector('.tramp-options');
photoDrop = document.querySelector('.photo-options');
dropDowns = document.querySelector('#dropdowns');
// check adventure dropdown choice
if (field.id === "adventure" && adventureForm.selectedIndex === 0) {
dropDowns.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 1) {
dropDowns.classList.remove('hidden');
charterDrop.classList.remove('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 2) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.remove('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 3) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
m
I have made this form that currently has onblur JavaScript validation and additionally I am to make it dynamic so that when a user selects an option from a dropdown option form a div is shown with corresponding options. But at the moment that code I have written for this is looking pretty repetitive and suffering from 'code smell'.
Is there a better way to do this? I am thinking a for loop that iterates through the dropdown options but am not 100% sure how to put this in place?
```
adventureForm = document.querySelector('#adventure');
charterDrop = document.querySelector('.charter-options');
mountainDrop = document.querySelector('.mountain-lake-options');
trampDrop = document.querySelector('.tramp-options');
photoDrop = document.querySelector('.photo-options');
dropDowns = document.querySelector('#dropdowns');
// check adventure dropdown choice
if (field.id === "adventure" && adventureForm.selectedIndex === 0) {
dropDowns.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 1) {
dropDowns.classList.remove('hidden');
charterDrop.classList.remove('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 2) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.remove('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (field.id === "adventure" && adventureForm.selectedIndex === 3) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
m
Solution
There are definitely improvements. Let us start with some basics.
Begin your file with
This avoids a class of nasty bugs that easily creep into JavaScript programs.
Do not use global variables. Parameterization is generally the best option but in this case we can avoid polluting the global namespace by simply wrapping our code in an immediately invoked function expression (IIFE).
Continuing, there is indeed a lot of repetition as you have correctly recognized. Fortunately, JavaScript makes it easy to remove this kind of duplication while at the same time making our code more declarative. Let's start with some simple refactoring
Now let us extract the code under
gets us to this so far
Note that we can factor out the common test into an early return.
There is a lot more we can do here. We could simplify with a switch statement, but I think a cleaner approach would be to use an object.
Let us convert our imperative conditional code into a mapping that declares which elements should be visible and which elements need to be hidden for each specific option.
Not only is this more readable, as we can see clearly which states include and exclude which components. Now
Here is what we end up with
There is definitely room for improvement, but I would argue that this is a step in the right direction.
Looking over the linked html, each possible value of
Begin your file with
'use strict';This avoids a class of nasty bugs that easily creep into JavaScript programs.
Do not use global variables. Parameterization is generally the best option but in this case we can avoid polluting the global namespace by simply wrapping our code in an immediately invoked function expression (IIFE).
(function () {
var elements;
var i = 0;
var isError = false;
var errorMessage;
var correctMessage = '✓';
var errorSpan;
...
}());Continuing, there is indeed a lot of repetition as you have correctly recognized. Fortunately, JavaScript makes it easy to remove this kind of duplication while at the same time making our code more declarative. Let's start with some simple refactoring
function hide(element) {
element.classList.add('hidden');
}
function show(element) {
element.classList.remove('hidden');
}Now let us extract the code under
// check adventure dropdown choicegets us to this so far
function checkAdventureDropdown(option) {
if (option.id !== 'adventure') {
return;
}
if (adventureForm.selectedIndex === 0) {
dropDowns.classList.add('hidden');
} else if (adventureForm.selectedIndex === 1) {
dropDowns.classList.remove('hidden');
charterDrop.classList.remove('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 2) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.remove('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 3) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.remove('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 4) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.remove('hidden');
}
}Note that we can factor out the common test into an early return.
There is a lot more we can do here. We could simplify with a switch statement, but I think a cleaner approach would be to use an object.
Let us convert our imperative conditional code into a mapping that declares which elements should be visible and which elements need to be hidden for each specific option.
var states = {
0: { hide: [dropDowns], show: [] },
1: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
2: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
3: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
4: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
}Not only is this more readable, as we can see clearly which states include and exclude which components. Now
checkAdventureDropdown becomes quite simple, with very few places for bugs to hide.function checkAdventureDropdown(option, states) {
if (option.id !== 'adventure') {
return;
}
var state = states[adventureForm.selectedIndex];
state.show.forEach(show);
state.hide.forEach(hide);
}Here is what we end up with
'use strict';
(function () {
var elements;
var i = 0;
var isError = false;
var errorMessage;
var correctMessage = '✓';
var errorSpan;
var adventureForm;
var charterDrop;
var mountainDrop;
var trampDrop;
var photoDrop;
var dropDowns;
var states = {
0: { hide: [dropDowns], show: [] },
1: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
2: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
3: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
4: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
if (option.id !== 'adventure') {
return;
}
function checkAdventureDropdown() {
var state = states[adventureForm.selectedIndex];
state.show.forEach(show);
state.hide.forEach(hide);
}
checkAdventureDropdown();
}());There is definitely room for improvement, but I would argue that this is a step in the right direction.
Looking over the linked html, each possible value of
adventureForm.selectedIndex corresponds to a meaningCode Snippets
'use strict';(function () {
var elements;
var i = 0;
var isError = false;
var errorMessage;
var correctMessage = '✓';
var errorSpan;
...
}());function hide(element) {
element.classList.add('hidden');
}
function show(element) {
element.classList.remove('hidden');
}function checkAdventureDropdown(option) {
if (option.id !== 'adventure') {
return;
}
if (adventureForm.selectedIndex === 0) {
dropDowns.classList.add('hidden');
} else if (adventureForm.selectedIndex === 1) {
dropDowns.classList.remove('hidden');
charterDrop.classList.remove('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 2) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.remove('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 3) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.remove('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 4) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.remove('hidden');
}
}var states = {
0: { hide: [dropDowns], show: [] },
1: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
2: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
3: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
4: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
}Context
StackExchange Code Review Q#142960, answer score: 2
Revisions (0)
No revisions yet.