DEV Community

Discussion on: Converting a React component to TypeScript

Collapse
lukeshiru profile image
LUKESHIRU

I have a few suggestions to improve this, I put them directly in code and added comments:

import Add from "@material-ui/icons/Add";
import type { ChangeEventHandler, MouseEventHandler, FC } from "react";
import { useState } from "react";

interface IAddWordProps {
    readonly onWordAdd?: (value: string) => void;
}

// ⚠️ You can use the `FC` type here
export const AddWord: FC<IAddWordProps> = ({ onWordAdd }) => {
    const [newWord, setNewWord] = useState("");

    // ⚠️ You could inline this and skip all the typing
    const onAddClicked: MouseEventHandler<HTMLButtonElement> = () => {
        // ⚠️ You can use `?.` to run this function only when is defined
        // ⚠️ You don't need a ref, you're already tracking the value with `newWord`
        onWordAdd?.(newWord);
        setNewWord("");
    };

    // ⚠️ You could inline this and skip all the typing
    const onChange: ChangeEventHandler<HTMLInputElement> = ({
        currentTarget: { value }
    }) => setNewWord(value);

    // ⚠️ You don't need `<Fragment></Fragment>`, you can use `<></>`
    return (
        <>
            <input
                name="new"
                onChange={onChange}
                pattern="[Bb]anana|[Cc]herry"
                placeholder="Add word..."
                required
                type="text"
                value={newWord}
            />
            <button
                onClick={onAddClicked}
                // ⚠️ You don't need a state for this, you can just put the logic here
                disabled={newWord.length < 2 || /\s/.test(newWord)}
            >
                {/* ⚠️ You can make `Add` self-closing */}
                <Add />
            </button>
        </>
    );
};

export default AddWord;
Enter fullscreen mode Exit fullscreen mode

Cheers!

Collapse
mbarzeev profile image
Matti Bar-Zeev Author

BTW, I'm not using FC on purpose ;)

Collapse
mbarzeev profile image
Matti Bar-Zeev Author

... and another thing - I like to keep my component's state as such, even though the "disabled" can be calculated on render. I find it more convenient if later on I would like to introduce a prop which initialize that state and it is much more readable IMO.

Thread Thread
lukeshiru profile image
LUKESHIRU

If you introduce a new prop, that prop can be added to the logic of disabled. If you want to keep it readable you can have it in a const instead of having it inline. What's important is to avoid using state when you don't need it, and in this case disabled can be inferred from other state+props, so no need for a state for it and a reducer just to keep that in sync. I mean you have:

// This
const Example = ({ logic }) => {
    const [disable, setDisable] = useState(true);
    useEffect(() => setDisable(logic), [logic]);
    return <input disabled={disable} />;
};

// vs this:
const Example = ({ logic }) => {
    return <input disabled={logic} />;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
mbarzeev profile image
Matti Bar-Zeev Author

Great code review 👍 thanks!
This this "sandbox" game is being tweaked by me as time goes by I tend to focus on the specific reasons for modifying it, and so some original code can surely use some modern react practices :)

Collapse
constantiner profile image
Konstantin Kovalev

I'd suggest not to use FC< IAddWordProps > (or FunctionComponent<IAddWordProps>) here because of this type supposes component also accepts children (in this case not). Use VoidFunctionComponent<IAddWordProps> (or VFC<IAddWordProps>) instead.

Collapse
lukeshiru profile image
LUKESHIRU

I have a kinda personal rule to only use JSX with components that can take children, if they don't, then they are should be just functions instead, because I like my components to be as flexible and predictable as you were using the underlying native elements directly. I know there are a few native elements such as input that don't take children, but those are just a few exceptions.

Don't get me wrong, you're correct, VFC is a better type if you don't use children, but I personally prefer to always use children if I'm using JSX.

Thread Thread
constantiner profile image
Konstantin Kovalev

But for this particular component even if you pass some children, they will attach nowhere. So passing them is useless. Why not restrict them then? Or I missed the point?

Thread Thread
lukeshiru profile image
LUKESHIRU

For this particular component, children could go just next to the Add icon, like: <Add />{children}, or make it optional, like: {children ?? <Add />}, but yup, as it's being used right now, VFC is a better option (I mainly explained the reason why I personally default to FC).