0

I have created a Select Dropdown component with its state abstracted in the reducer. I am just updating this collective state using reducer actions and handling the side effects via useEffect but there are too many useEffects now and made me wonder is this the right approach at all? Also, when I am using reducer actions to update this collective state, are these changes qualified to be called side-effects at all? Sharing two code blocks below:

  1. List.jsx with logic of the component
  2. List.reducer.js with reducer logic in it.

List.jsx

import React, { useRef, useCallback, useReducer, useEffect } from "react"
import PropTypes from "prop-types"
import "./list.style.sass"
import ListItem from "../../pure-components/list-item/list-item.jsx"
import TextField from "../../pure-components/text-field/text-field.jsx"
import PrimaryButton from "../../pure-components/primary-button/primary-button.jsx"
import SecondaryButton from "../../pure-components/secondary-button/secondary-button.jsx"
import { useFilterListSearch, useClickOutside, useKeyPress } from "../../hooks"
import { nanoid } from "nanoid"
import { reducer, initialState, ACTION } from "./list.reducer.js"

const List = ({
    list,
    searchable,
    button,
    defaultItem,
    searchWhichKeys,
    onSelect,
}) => {
    /**
     * State of this component
     */
    const [state, dispatch] = useReducer(reducer, initialState)
    const filteredList = useFilterListSearch(
        list, // Default list
        state.searchQuery,
        searchWhichKeys
    )

    const listRef = useRef(null)
    const escPress = useKeyPress("Escape")

    const closeList = useCallback(
        (event) => {
            if (event) return
            dispatch({ type: ACTION.HIDE_LIST })
        },
        [state.listVisibility]
    )

    useClickOutside(listRef, closeList)

    const onItemClicked = (item) => {
        dispatch({ type: ACTION.HIDE_LIST })
        dispatch({ type: ACTION.UPDATE_USER_SELECTION, payload: item })
    }

    const callbackOnButtonClick = useCallback(
        (buttonPressedState, fn) => {
            const buttonState = buttonPressedState
                ? ACTION.SHOW_LIST
                : ACTION.HIDE_LIST

            console.count([
                buttonState,
                "because button-pressed is",
                buttonPressedState,
                fn,
                state.buttonUID,
            ])
            dispatch({ type: buttonState })
        },
        [state.buttonUID]
    )

    const renderSelectButton = useCallback(() => {
        const buttonProps = {
            displayTooltip: true,
            label: state.userSelection
                ? state.userSelection.option
                : button?.label,
            icon: button?.icon,
            callback: callbackOnButtonClick,
            keepPressedAfterClick: true,
        }

        return button.type === "primary" ? (
            <PrimaryButton key={state.buttonUID} {...buttonProps} />
        ) : (
            <SecondaryButton key={state.buttonUID} {...buttonProps} />
        )
    }, [state.buttonUID])

    const resetButonState = () => {
        /**
         * Resets the button's state whenever the list's visibility
         * is transitoned from shown to hidden. We are not resetting
         * the UID in case of state transition from hidden to
         * visible as it's resetting button's state on each click
         *
         * Therefore, if list is visible we will not change the UID only when it
         * is made to hide, we will reset the button state as well
         */
        if (state.listVisibility) return
        dispatch({ type: ACTION.UPDATE_BUTTON_UID, payload: nanoid() })
    }

    const resetToDefaultList = () => {
        /**
         * Whenever, we open the list we want it to be the default list
         * not filtered one from the previously applied filter (if any)
         * Therefore, we will reset it to default whenever it is closed.
         * Which is equivalent of setting the user query to null
         */
        if (state.listVisibility) return
        dispatch({ type: ACTION.RESET_SEARCH_QUERY })
    }

    useEffect(() => {
        /**
         * Whenever the default item provided changes, we update
         * the userSelectin state with default selection
         */
        dispatch({ type: ACTION.UPDATE_USER_SELECTION, payload: defaultItem })
    }, [defaultItem])

    useEffect(() => {
        /**
         * Initially we fill the dropdown list with the items in list prop and
         * again do the same whenever this provided list changes
         */
        dispatch({ type: ACTION.UPDATE_LIST, payload: list })
    }, [list])

    useEffect(() => {
        /**
         * We update the ref list state whenever the reference to the list of
         * this component changes
         */
        dispatch({ type: ACTION.UPDATE_LIST_REF, payload: listRef })
    }, [listRef])

    useEffect(() => {
        /**
         * Reset the pressed state fof the button
         */
        resetButonState()
        /**
         * Resets to the default list
         */
        resetToDefaultList()
    }, [state.listVisibility])

    useEffect(() => {
        if (state.userSelection === "") return
        /**
         * Whenever user selects an item we call onSelect callback and
         * pass this user selected item as an argument of it
         */
        onSelect(state.userSelection)
    }, [state.userSelection])

    useEffect(() => {
        /**
         * Whenever custom hook filtered list changes we'll sync
         * the resultant list with component's state
         */
        dispatch({ type: ACTION.UPDATE_LIST, payload: filteredList })
    }, [filteredList])

    useEffect(() => {
        /**
         * If Esc is pressed hide the list else do nothing
         */
        if (!escPress) return
        dispatch({ type: ACTION.HIDE_LIST })
    }, [escPress])

    return (
        <div className="List" ref={listRef}>
            <div className="list-select">{renderSelectButton()}</div>

            {state.listVisibility ? (
                <div className="list-container">
                    {searchable ? (
                        <div className="list-header">
                            <TextField
                                name="search"
                                placeholder="Search..."
                                onChangeCallback={(event) => {
                                    dispatch({
                                        type: ACTION.UPDATE_SEARCH_QUERY,
                                        payload: event?.target?.value,
                                    })
                                }}
                            />
                        </div>
                    ) : null}
                    <div className="list-body">
                        {state.list.length
                            ? state.list.map((item) => {
                                return (
                                    <ListItem
                                        key={item.id}
                                        item={item}
                                        isActive={
                                            state.userSelection?.id ===
                                              item?.id
                                        }
                                        onClickCallback={onItemClicked}
                                    />
                                )
                            })
                            : null}
                    </div>
                </div>
            ) : null}
        </div>
    )
}

List.propTypes = {
    list: PropTypes.array.isRequired,
    searchable: PropTypes.bool,
    button: PropTypes.object.isRequired,
    defaultItem: PropTypes.object,
    searchWhichKeys: PropTypes.array,
    onSelect: PropTypes.func,
}

List.defaultProps = {
    list: [
        {
            id: nanoid(),
            option: "Anything",
        },
    ],
    searchable: false,
    button: {
        type: "secondary",
        icon: null,
        label: "Button",
    },
    defaultItem: null,
    searchWhichKeys: ["option"],
    onSelect: () => {},
}

export default List

List.reducer.js

/**
 * Initial state
 */
export const initialState = {
    listVisibility: false,
    userSelection: "",
    searchQuery: "",
    buttonUID: "",
    list: [],
    refs: {
        list: null,
    },
    error: {
        status: false,
        message: "",
    },
}
/**
 * Reducer function
 */
export const reducer = (state, action) => {
    switch (action.type) {
    case ACTION.SHOW_LIST:
        return { ...state, listVisibility: true }

    case ACTION.HIDE_LIST:
        return { ...state, listVisibility: false }

    case ACTION.UPDATE_USER_SELECTION:
        return { ...state, userSelection: action.payload }

    case ACTION.UPDATE_SEARCH_QUERY:
        return { ...state, searchQuery: action.payload }

    case ACTION.RESET_SEARCH_QUERY:
        return { ...state, searchQuery: "" }

    case ACTION.UPDATE_LIST:
        return { ...state, list: action.payload }

    case ACTION.UPDATE_BUTTON_UID:
        return { ...state, buttonUID: action.payload }

    case ACTION.UPDATE_LIST_REF:
        return {
            ...state,
            refs: {
                ...state.refs,
                list: action.payload,
            },
        }

    case ACTION.THROW_ERROR:
        return {
            ...state,
            error: {
                ...state.error,
                status: true,
                message: action.payload,
            },
        }

    case ACTION.CLEAR_ERROR:
        return {
            ...state,
            error: { ...state.error, status: false, message: "" },
        }

    default:
        throw new Error("Problematic Action: ", action)
    }
}
/**
 * Reducer Actions
 */
export const ACTION = {
    SHOW_LIST: "show-list",
    HIDE_LIST: "hide-list",
    UPDATE_USER_SELECTION: "update-user-selection",
    UPDATE_SEARCH_QUERY: "update-search-query",
    RESET_SEARCH_QUERY: "reset-search-query",
    UPDATE_LIST: "update-list",
    UPDATE_BUTTON_UID: "update-button-uid",
    UPDATE_LIST_REF: "update-list-ref",
    THROW_ERROR: "throw-error",
    CLEAR_ERROR: "clear-error",
}


Abhinay
  • 11
  • 4
  • That's a lot of code to review – if there's no actual _bug_ with the code, it belongs on the Code Review SE. That said, I can eyeball a bug with the `UPDATE_LIST_REF` effect – the ref object itself will never change, so that effect will only ever be fired once (and mutating a ref-box won't cause an update by itself anyway). – AKX Aug 22 '22 at 11:00
  • (Also, what's up with that "button UID"? That's a code smell right there...) – AKX Aug 22 '22 at 11:01
  • Its a trick to reset the button state. Regarding this how can a parent reset a child component without resetting it's key? – Abhinay Aug 22 '22 at 11:04
  • What internal state would a button have that needs to be reset? – AKX Aug 22 '22 at 11:05
  • 'Pressed' state, I want the button to remain pressed as long as the list is visible. Button, this list all are part of our internal component's library. So want to keep these components as generalised as possible. – Abhinay Aug 22 '22 at 11:07
  • Well, it sounds like the "pressed" visual state should be controlled by this component then instead of a "keep pressed after click" internal state in the button itself. (In that sense, that button component is hardly "as generalised as possible" already.) – AKX Aug 22 '22 at 11:10
  • Also, a `useKeyPress` hook probably shouldn't be stateful either; a keypress is a transient event. A better signature would be `useKeyPress(keyName, onKeyPressed)`... – AKX Aug 22 '22 at 11:14
  • Not in my books. If you wanted a "smart" button that could stay pressed after click, that would be a separate stateful component that wraps the "dumb" stateless visual button component. – AKX Aug 22 '22 at 11:15
  • Okay, getting your points. Thanks for these valuable feedback, will rectify these issues in my code :) – Abhinay Aug 22 '22 at 11:18
  • That makes sense! Wrapping a dumb stateless component within the statefull components. But will it not be then similar to managing the same states from parent itself? Now I am confused. Why this extra layer when there's hardly a difference (or I may have not understood your point correctly) – Abhinay Aug 22 '22 at 11:22
  • There's exactly the difference that you'd want this dropdown list component to be managing the "pressed" visual state of a dumb button component instead of having to unmount/remount a smart component to have it change its internal state. – AKX Aug 22 '22 at 11:24
  • And also what about my original question? – Abhinay Aug 22 '22 at 11:24
  • "unmount/remount a smart component to have it change its internal state" Oh, now I see! – Abhinay Aug 22 '22 at 11:26
  • As for your original question, well, I think this has code smells all around; getting rid of the "button UID" stuff and making the keypress hook callback-shaped will likely simplify things a lot. – AKX Aug 22 '22 at 11:31
  • Thanks for your suggestion! Will definitely make the changes. – Abhinay Aug 22 '22 at 11:35
  • Oh, and eyeballing a bit more: don't keep a copy of `filteredList` in your state, there's no need to. – AKX Aug 22 '22 at 11:36
  • Finally: none of the actions in your reducer modify more than one field of the state, which means you don't need the reducer at all. – AKX Aug 22 '22 at 11:37
  • I thought the general rule of state complexity in react is the no. of state fields in a component apart from the complexity in the structure of any given state field? From my POV, more the state variables/fields in the component, better to use reducer. Also, at the same time, not discarding that the complexity of state is one of the factor of using reducer which I guess is your point. My state may not be complex but have significant number of variables IMO that warrants a reducer (and on more personal note, using reducer makes me feel, I've written a more readable and comprehending code) – Abhinay Aug 22 '22 at 11:53
  • It's not quite the number of state atoms – after all, you could just merrily stuff all of your state into `const [state, setState] = ...` and voilà, a single state atom, no need for reducers, hey? (and anyway, if you squint, that's what your reducer state is) – but whether those atoms interact a lot when they're acted upon. In this component, they don't seem to, at all really. – AKX Aug 22 '22 at 11:58
  • Oh now I get it, the state of this component is more like a gas than solid and reducers work best when we have a solid state where interaction between individual atoms is pretty strong giving rise to a rigid structure. Am I correct in interpreting your point? – Abhinay Aug 22 '22 at 12:03
  • Something like that, I suppose. "state atom" is just another word for what you call "state field", but in any case one `const [x, setX] = useState()` invocation. Anyway, I see your component would simplify to something like https://gist.github.com/akx/4f7cf103be7e5895a1a2cd9e34bff999 if you have a dumb button component. – AKX Aug 22 '22 at 12:07
  • Thanks for sharing the simplification. I see, so - correct me if I am wrong - to summarise: 1. Dumb/Stateless components should not try to behave smart. This smartness should either come from wrapper layer or parent itself. Dumb components should simply use the state passed to them and not be manipulating them. No point in making an active state out of something which inherently have passive state.. 2. Using reducer is beneficial in case there's high degree of inter-dependness / overlapping among state atoms – Abhinay Aug 22 '22 at 12:23
  • But what about code readability and manageability in case there are not so overlapping atoms but many atoms nonetheless, and we are not using reducers? – Abhinay Aug 22 '22 at 12:23
  • (1) It's many times (not always, not never) worth it to separate a presentational/dumb/stateless component from a smarter/stateful one, especially for composition like this. (2) Yes – if an UI action will affect multiple atoms, and especially if actions tend to need to read other atoms to affect others, useReducer can be cleaner. (3) If you're truly worried about readability, add comments and/or wrap things into custom hooks. – AKX Aug 22 '22 at 12:34

0 Answers0