Your idea about my functional component code

Hello,

I’ve changed my class-based component to functional.
I will be pleased if you let me know your idea about the code or if anywhere is wrong.

import { Redirect } from "react-router-dom";
import React, { useState } from "react";

export function ImageUpload1() {

    const [redirect, setRedirect] = useState(null);
    const [selectedFile, setSelectedFile] = useState(null);
    const [description, setDescription] = useState(null);
    const [uploadResult, setUploadResult] = useState(null);
    const [selectedFileName, setSelectedFileName] = useState(null);
    const [fileIdList, setFileIdList] = useState([]);


    useEffect(

        
        const getList = () => {
            fetch('api/Image')
                .then(response => response.json())
                .then(data => setFileIdList({ fileIdList: data }));
        };
    );

    const onFileChange = event => {

       
        
        setSelectedFile(event.target.files[0]);
        setSelectedFileName(event.target.files[0]);
    };

    const onDescriptionChange = event => {

        setDescription(event.target.value);
    };
    const onFileUpload = async () => {

        const formData = new FormData();

        formData.append(
            "ImageData.File",
            setSelectedFile,
            setSelectedFileName
        );



        fetch('/api/Image', {
            method: 'POST',
            body: formData
        }).then(resposne => resposne.json())
            .then(data => {
                console.log(data);
              
                setUploadResult: "File" + data.fileName + "successfully uploaded";
                getList();
            });
    }

    const onNavToEditor = async () => {

        //this.onFileChangeName();

        //this.setState({ redirect: "/ImageEditor" });
        //    this.setState({ selectedFileName: });

        setRedirect( "/ImageEditor");

    };
    **const listItems = () => {**
**        const listItems = this.state.fileIdList.map((item) =>**
**            <div key={item.imageId.toString()}>**
**                <img src={"/api/Image/DownloadImage/" + item.imageId}**
**                    alt={item.fileName}**
**                    className="img-thumbnail"**
**                    height="100" width="100" />**
**            </div>**
**        );**
        return (<div>{listItems}</div>);
    };

    return (

        <div>
            <div style={mystyle} onClick={NavToEditor} />
            <h1>File Upload Demo</h1>
            <div >{uploadResult} onClick={NavToEditor}</div>
            <div>
                <input type="file" onChange={FileChange} />
                <input type="text" **value={this.state.value}** onChange={DescriptionChange} />
                <button onClick={FileUpload}>
                    Upload!
                    </button>

            </div>

            <div onClick={NavToEditor}>

                {listItems()}

            </div>

regards,
Saeed

Well a couple things:

  1. I would look for a way to use some custom hooks here (don’t ask exactly where, I’m looking at this from a 1st round code review perspective).
  2. I would take the fetch handler out (prob find a way to incorporate into one of the hooks).
  3. onFileChange, your writing the same data to 2 different states, this prob can be condensed into one (again, custom hook)
  4. Click handler on a div, there’s no way to focus on that AND even if there was, that’s an accessibility no-no.
  5. A couple of the onClick handlers are inside of the markup and not attached to an element.
  6. No labels on inputs, that would be needed.
  7. This should probably be wrapped in a form with legend / fieldset and an on submit handler on the button.
  8. List items should be ‘li’ wrapped in an ul tag.
  9. image src, I’d recommend using string template literals, also alt tags value should be optional (yeah I know what your thinking, but while all images NEED an alt tag, they done NEED it to be filled in depending on WHAT they are for), and I would use something other than file name.
  10. I FEEL like useEffect the way it is currently set up can either cause an infinite loop, or at best a memory leak.

I would def look into a way to get the fetch handlers out of the view. This is just a quick once over. Normally I’d test in in browser but, yeah I’m not “working”, lol.

And yes, I copied my reply from the “other” place, 'cause lazy…

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.