This component is meant to take in an array of available options and provide the user an easy way to choose multiple options and filter on the available options. Each time the selected options are changed/updated, those options are console logged.
I'm hoping that someone could provide any criticism on my current code. I want to make sure that the code is as simple and clean as it could be, while keeping all logic outside of the JSX. Photo of component at bottom of post. If you notice any complexities that could be replaced with some alternative design, i'd be interested to know.
import React, { useState, useEffect } from 'react';
import { Icon, MyInput } from '@mylib/library';
import './MultiSelection.scss';
export default function MultiSelection({
initialOptions=['A', 'B', 'C', 'D', 'Aaron', 'Britney', 'Carter'],
componentLabel='Select Contract Types',
availableLabel='Available Types',
selectedLabel='Selected Types',
filterLabel='Filter available Types',
showFilter=true,
}) {
const [options, setOptions] = useState([]);
const [availableOptions, setAvailableOptions] = useState([]);
const [selectedOptions, setSelectedOptions] = useState([]);
const [availableFilterValue, setAvailableFilterValue] = useState('');
// Convert options into a manageable structure
// on initial render
useEffect(() => {
let tempOptions = [];
initialOptions.forEach(option => tempOptions.push({'text': option, 'isAvailable': true, 'shouldShow': true}));
setOptions(tempOptions);
}, []);
// Separates options into a available and selected array
// in order to keep logic out of the JSX
useEffect(() => {
console.log('Build available and selected options');
const availableOptionsText = [];
const selectedOptionsText = [];
const available = options.filter(option => option.isAvailable && option.shouldShow);
const selected = options.filter(option => !option.isAvailable);
available.forEach(option => availableOptionsText.push(option.text));
selected.forEach(option => selectedOptionsText.push(option.text));
setAvailableOptions(availableOptionsText);
setSelectedOptions(selectedOptionsText);
}, [options]);
const updateStateOfOption = clickedOption => {
const tempOptions = [...options];
tempOptions.forEach(option => {
if(option.text === clickedOption){
option.isAvailable = !tempOptions.find(option => option.text === clickedOption).isAvailable
}
});
setOptions(tempOptions);
}
const handleFilterOnChange = e => {
console.log('Hit handleFilterOnChange');
setAvailableFilterValue(e.target.value);
}
const handleFilterOnBlur = e => {
console.log('Hit handleFilterOnBlur');
setAvailableFilterValue(e.target.value);
}
// Modify which options' shouldShow property depending on filter
// Only the available options will care about the shouldShow property
useEffect(() => {
const tempOptions = [...options];
tempOptions.forEach(option => {
if(availableFilterValue === ''){
option.shouldShow = true;
} else {
if(!option.text.toLowerCase().startsWith(availableFilterValue.toLocaleLowerCase())){
option.shouldShow = false;
} else {
option.shouldShow = true;
}
}
});
if(tempOptions.length !== 0){
setOptions(tempOptions);
}
}, [availableFilterValue]);
const row = (text, key, isAvailableColumn) => {
return(
<div key={`${isAvailableColumn ? 'available' : 'selected'}-button-row-${key}`} className='button-row-container' onClick={() => updateStateOfOption(text)}>
<Icon id='add' iconClass={`${isAvailableColumn ? 'icon-add' : 'icon-trashcan'}`}/>
<div>{text}</div>
</div>
)
}
return(
<div className='multi-selection'>
<label className='dropdown__floatingLabel multi-selection-label'>{componentLabel}</label>
{showFilter &&
<div className='-row page-row'>
<div className='col col-xs-6 col-sm-6 col-md-6 col-lg-6'>
<MyInput
name='availableTypesFilter'
id='availableTypesFilter'
autofocus={false}
fieldValue={availableFilterValue ? availableFilterValue : ''}
errorMessage={''}
label={filterLabel}
allowAutoComplete={false}
changed={handleFilterOnChange}
onBlur={handleFilterOnBlur}
/>
</div>
</div>
}
<div className={`label-container-${showFilter ? 'with-filter' : 'without-filter'}`}>
<p className='dropdown__floatingLabel'>{availableLabel}</p>
<p className='dropdown__floatingLabel'>{selectedLabel}</p>
</div>
<div className='options-container'>
<div className='available-options'>
{availableOptions.map((option, key) => row(option, key, true))}
</div>
<div className='selected-options'>
{selectedOptions.map((option, key) => row(option, key, false))}
</div>
</div>
</div>
)
}
1 Answer 1
There are a few places in the code where you have an array and you want to construct another array from it. Rather than forEach followed by push, it's more appropriate to use Array.prototype.map to transform one array into another. For example:
useEffect(() => {
let tempOptions = [];
initialOptions.forEach(option => tempOptions.push({ 'text': option, 'isAvailable': true, 'shouldShow': true }));
setOptions(tempOptions);
}, []);
can turn into
useEffect(() => {
const newOptions = initialOptions.map(text => ({ text, isAvailable: true, shouldShow: true }));
setOptions(newOptions);
}, []);
As you can see above, properties that are valid identifiers don't need to be quoted (and probably shouldn't, to cut down on noise), and by planning ahead with variable names, you can sometimes use shorthand properties rather than having to list both the key and its value. In addition, you should always use const to declare variables when possible - don't use let unless you wish to warn the reader of the code that you may have to reassign the variable later.
You can use .map in the second useEffect call too, to great effect:
useEffect(() => {
console.log('Build available and selected options');
const optionToText = option => option.text;
const availableOptionsText = options
.filter(option => option.isAvailable && option.shouldShow)
.map(optionToText);
const selectedOptionsText = options
.filter(option => !option.isAvailable)
.map(optionToText);
setAvailableOptions(availableOptionsText);
setSelectedOptions(selectedOptionsText);
}, [options]);
In updateStateOfOption, you are mutating the isAvailable property of one of the options array objects:
const updateStateOfOption = clickedOption => {
const tempOptions = [...options];
tempOptions.forEach(option => {
if (option.text === clickedOption) {
option.isAvailable = !tempOptions.find(option => option.text === clickedOption).isAvailable
}
});
setOptions(tempOptions);
}
Spreading an array only creates a shallow copy - each element in the original array is still a reference to one of the elements in the new array, so mutating an element in the new array changes an element in the old array too. Since this is React, such mutation should be avoided. Instead, use findIndex to find the index of the matching option, then create a new array by spreading the parts of the array before the option, then declaring a new (modified) object, then spreading the parts of the array after the option:
const updateStateOfOption = clickedOption => {
const clickedIndex = options.findIndex(option => option.text === clickedOption);
const newOptions = [
...options.slice(0, clickedIndex),
{ ...options[clickedIndex], isAvailable: !options[clickedIndex].isAvailable },
...options.slice(clickedIndex + 1),
];
setOptions(newOptions);
}
You have the same issue in the next useEffect too. This time, you can change every element of the array by using .map and spreading the previous option into an object, then assigning the new shouldShow property:
useEffect(() => {
const newOptions = options.map(option => ({
...option,
shouldShow: availableFilterValue === '' || option.text.toLowerCase().startsWith(availableFilterValue.toLocaleLowerCase())
}));
if (newOptions.length !== 0) {
setOptions(newOptions);
}
}, [availableFilterValue]);
You can use || instead of the conditional operator here, if you want:
fieldValue={availableFilterValue ? availableFilterValue : ''}
can be
fieldValue={availableFilterValue || ''}