Wątek przeniesiony 2023-07-06 10:44 z JavaScript przez Riddle.

Ocena formularza rejestracji

0

Zacząłem się uczyć Reacta razem z TypeScript i tworzę obecnie aplikację typu CRUD. na razie stworzyłem rejestrację, oczywiście ten kod działa, tylko mam prośbę aby ktoś bardziej doświadczony sprawdził ten kod i wskazał co można w tym kodzie poprawić tak abym tych błędów więcej nie popełniał. Chodzi mi o to co można w tym kodzie zmienić na lepsze i czy można ten kod przenieść do innych miejsc żeby było bardziej czytelnie. Z góry dzięki

import {FC, useState} from "react";
import axios from "axios";

const AuthRegister: FC = () => {
    const [formValues, setFormValues] = useState({
        first_name: "",
        last_name: "",
        email: "",
        password: "",
    });
    const [errors, setErrors] = useState({
        first_name: "",
        last_name: "",
        email: "",
        password: "",
    });
    const [message, setMessage] = useState(false);
    const [showMessage, setShowMessage] = useState(false);

    const onChange = (e: React.ChangeEvent<HTMLInputElement>) => {
        const {name, value} = e.target;
        setFormValues({...formValues, [name]: value});
    };

    const storeUser = async (event: React.FormEvent<HTMLFormElement>) => {
        event.preventDefault();
        try {
            const response = await axios.post("http://store.localhost/api/auth/register", formValues);
            setFormValues({
                first_name: "",
                last_name: "",
                email: "",
                password: "",
            });
            const message = response.data.message;
            setMessage(message);
            setShowMessage(true);

        } catch (error: any) {
            if (error.response && error.response.status === 422) {
                setErrors(error.response.data.errors);
            } else {
                console.log(error);
            }
        }
    }
    return (
        <div className="flex justify-center items-center h-screen bg-teal-100">
            <form className="bg-teal-600 rounded-lg p-8 w-96" onSubmit={storeUser}>
                <div className="text-white text-center">
                    <h1 className="text-4xl mb-8">Registration</h1>
                </div>
                <div className="text-center">
                    {showMessage && (
                        <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md">
                            <strong>{message}</strong>
                        </div>
                    )}
                </div>
                <div className="mb-6 mt-8">
                    <label htmlFor="" className="block mb-2 text-sm font-medium text-white">Your first name</label>
                    <input
                        type="text"
                        id="first_name"
                        name="first_name"
                        value={formValues["first_name"]}
                        onChange={onChange}
                        className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"/>
                    {errors.first_name && <span className="text-sm text-red-400"><strong>{errors.first_name[0]}</strong></span>}
                </div>
                <div className="mb-6 mt-8">
                    <label htmlFor="email" className="block mb-2 text-sm font-medium text-white">Your last name</label>
                    <input type="text"
                           id="last_name"
                           name="last_name"
                           value={formValues['last_name']}
                           onChange={onChange}
                           className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"/>
                    {errors.last_name && <span className="text-sm text-red-400"><strong>{errors.last_name[0]}</strong></span>}
                </div>
                <div className="mb-6 mt-8">
                    <label htmlFor="email" className="block mb-2 text-sm font-medium text-white">Your email</label>
                    <input type="email"
                           id="email"
                           name="email"
                           value={formValues['email']}
                           onChange={onChange}
                           className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"/>
                    {errors.email && <span className="text-sm text-red-400"><strong>{errors.email[0]}</strong></span>}
                </div>
                <div className="mb-6">
                    <label htmlFor="password" className="block mb-2 text-sm font-medium text-white">Your password</label>
                    <input type="password"
                           id="password"
                           name="password"
                           value={formValues['password']}
                           onChange={onChange}
                           className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"/>
                    {errors.password && <span className="text-sm text-red-400"><strong>{errors.password[0]}</strong></span>}
                </div>
                <button type="submit" className="text-white bg-sky-500 focus:ring-4 focus:outline-none focus:ring-blue-300 font-medium rounded-lg text-sm w-full sm:w-auto px-5 py-2.5 text-center">Sign up</button>
            </form>
        </div>
    );
};


export default AuthRegister;
1

Jak ja nie lubię syntaxu Reactowego... my eyes 😣

1

aż prosi sie zeby wrzucic ten fragment jako osobny komponent :v

<div className="mb-6 mt-8">
    <label htmlFor="" className="block mb-2 text-sm font-medium text-white">Your first name</label>
    <input
        type="text"
        id="first_name"
        name="first_name"
        value={formValues["first_name"]}
        onChange={onChange}
        className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"/>
    {errors.first_name && <span className="text-sm text-red-400"><strong>{errors.first_name[0]}</strong></span>}
</div>

i potem powtórzyc go 4 razy dla first_name, last_name, email, password

0

A czy Api można jakoś wydzielić z tego kodu do innego pliku np. RegisterApi, oraz hooki do innego pliku też ?

0
Schadoow napisał(a):

aż prosi sie zeby wrzucic ten fragment jako osobny komponent :v
i potem powtórzyc go 4 razy dla first_name, last_name, email, password

W taki sposób to zrobiłem to jest kod w pliku RegisterForm.tsx

import {FC} from "react";

const RegisterForm: FC<{
        label: string;
        id: string;
        name: string;
        value: string;
        onChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
        errors: string[] | null;
    }> = ({label, id, name, value, onChange, errors}) => {
        return (
            <div className="mb-6 mt-8">
                <label htmlFor={id} className="block mb-2 text-sm font-medium text-white">
                    {label}
                </label>
                <input
                    type="text"
                    id={id}
                    name={name}
                    value={value}
                    onChange={onChange}
                    className="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5"
                />
                {errors && <span className="text-sm text-red-400"><strong>{errors[0]}</strong></span>}
            </div>
        );
    }
;

export default RegisterForm;

A to kod w AuthRegister.tsx

import {FC, useState} from "react";
import axios from "axios";
import RegisterForm from "../common/RegisterForm";

const AuthRegister: FC = () => {
    const [formValues, setFormValues] = useState({
        first_name: "",
        last_name: "",
        email: "",
        password: "",
    });
    const [errors, setErrors] = useState({
        first_name: "",
        last_name: "",
        email: "",
        password: "",
    });
    const [message, setMessage] = useState(false);
    const [showMessage, setShowMessage] = useState(false);

    const onChange = (e: React.ChangeEvent<HTMLInputElement>) => {
        const {name, value} = e.target;
        setFormValues({...formValues, [name]: value});
    };

    const storeUser = async (event: React.FormEvent<HTMLFormElement>) => {
        event.preventDefault();
        try {
            const response = await axios.post("http://store.localhost/api/auth/register", formValues);
            setFormValues({
                first_name: "",
                last_name: "",
                email: "",
                password: "",
            });
            const message = response.data.message;
            setMessage(message);
            setShowMessage(true);

        } catch (error: any) {
            if (error.response && error.response.status === 422) {
                setErrors(error.response.data.errors);
            } else {
                // Obsługa innych błędów
                console.log(error);
            }
        }
    }
    return (
        <div className="flex justify-center items-center h-screen bg-teal-100">
            <form className="bg-teal-600 rounded-lg p-8 w-96" onSubmit={storeUser}>
                <div className="text-white text-center">
                    <h1 className="text-4xl mb-8">Registration</h1>
                </div>
                <div className="text-center">
                    {showMessage && (
                        <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md">
                            <strong>{message}</strong>
                        </div>
                    )}
                </div>
                <div>
                    <RegisterForm
                        label="Your first name"
                        id="first_name"
                        name="first_name"
                        value={formValues['first_name']}
                        onChange={onChange}
                        errors={Array.from(errors.first_name)}
                    />
                    <RegisterForm
                        label="Your last name"
                        id="last_name"
                        name="last_name"
                        value={formValues['last_name']}
                        onChange={onChange}
                        errors={Array.from(errors.last_name)}
                    />
                    <RegisterForm
                        label="Your email"
                        id="email"
                        name="email"
                        value={formValues['email']}
                        onChange={onChange}
                        errors={Array.from(errors.email)}
                    />
                    <RegisterForm
                        label="Your password"
                        id="password"
                        name="password"
                        value={formValues['password']}
                        onChange={onChange}
                        errors={Array.from(errors.password)}
                    />
                </div>
                <button type="submit" className="text-white bg-sky-500 focus:ring-4 focus:outline-none focus:ring-blue-300 font-medium rounded-lg text-sm w-full sm:w-auto px-5 py-2.5 text-center">Sign up</button>
            </form>
        </div>
    );
};


export default AuthRegister;
0

Tak na prawdę to nie ma tu za wiele żeby ocenić cokolwiek.

0

IMO git.
Jedyne co to zmieniłbym to nazwa RegisterForm bo to nie jest formularz a komponent inputa ostylowany :p wiec coś bardziej w "stylu" RegisterFormInput.

0
  1. Zmień nazwe komponentu z RegisterForm na FormInput - bardziej generyczna do zastosowania rowniez w innych formularzach.
  2. Zastanowilbym sie do uzycia jakiegos schema modelu (biblioteka np. Yup) oraz skorzystania z React Hook Form poniewaz brakuje walidacji frontowej.
  3. Nie stosuj takiego zapisu:
{showMessage && (
  <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md">
       <strong>{message}</strong>
   </div>
  )}

gdyż może to powodować potencjalne errory: https://medium.com/geekculture/stop-using-for-conditional-rendering-in-react-a0f7b96200f8
lepiej showMessage ? <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md"> <strong>{message}</strong> </div> : null

  1. Powyższy komponent message rowniez wydzielilbym do osobnego komponentu, ogolem warto powydzielac czesci kodu ktore sie powtarzaja do osobnych komponentow (wszystkie Message,Buttony,Inputy itp)
  2. W
const RegisterForm: FC<{
        label: string;
        id: string;
        name: string;
        value: string;
        onChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
        errors: string[] | null;
    }> = ({label, id, name, value, onChange, errors}) => {

stwórz sobie interface np interface IProps {} i przekaz je tak React.FC<IProps> bedzie duzo czytelniejsze.
6. Stworz osobny hook w ktorym bedziesz trzymal logike np usehandleStoreUser lub w tym wypadku w sumie lepiej wydzielic ta funkcje do jakiegos osobnego pliku w stylu registerActions.ts albo registerService.ts i exportowac ja
7. Zamiast import RegisterForm from "../common/RegisterForm"; poogarniaj absolute path żeby zamiast "../" robić "@/common/RegisterForm", w tym wypadku nie ma to az takiego znaczenia ale jak np bedziesz sie cofal folderowo "../../../../" no to juz wtedy poprawia czytelnosc ;]

0

@Riddle: czyli według Ciebie powinno tak zostać jak jest, bez wydzielania tego kodu do innych folderów czy plików?

0
phpowiec napisał(a):

@Riddle: czyli według Ciebie powinno tak zostać jak jest, bez wydzielania tego kodu do innych folderów czy plików?

Jeśli ten kod stałby się częścią większego projektu, to należałoby go pokroić na mniejsze kawałki.

Ale na ten moment ten kod jest za mały żeby wiedzieć w którym miejscu to najlepiej pociąć, więc tak długo jak ten kod jest taki mały, to lepiej na razie to zostawić.

0
RitzCarlton napisał(a):
  1. Zmień nazwe komponentu z RegisterForm na FormInput - bardziej generyczna do zastosowania rowniez w innych formularzach.
  2. Zastanowilbym sie do uzycia jakiegos schema modelu (biblioteka np. Yup) oraz skorzystania z React Hook Form poniewaz brakuje walidacji frontowej.
  3. Nie stosuj takiego zapisu:
{showMessage && (
  <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md">
       <strong>{message}</strong>
   </div>
  )}

gdyż może to powodować potencjalne errory: https://medium.com/geekculture/stop-using-for-conditional-rendering-in-react-a0f7b96200f8
lepiej showMessage ? <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md"> <strong>{message}</strong> </div> : null

  1. Powyższy komponent message rowniez wydzielilbym do osobnego komponentu, ogolem warto powydzielac czesci kodu ktore sie powtarzaja do osobnych komponentow (wszystkie Message,Buttony,Inputy itp)
  2. W
const RegisterForm: FC<{
        label: string;
        id: string;
        name: string;
        value: string;
        onChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
        errors: string[] | null;
    }> = ({label, id, name, value, onChange, errors}) => {

stwórz sobie interface np interface IProps {} i przekaz je tak React.FC<IProps> bedzie duzo czytelniejsze.
6. Stworz osobny hook w ktorym bedziesz trzymal logike np usehandleStoreUser lub w tym wypadku w sumie lepiej wydzielic ta funkcje do jakiegos osobnego pliku w stylu registerActions.ts albo registerService.ts i exportowac ja
7. Zamiast import RegisterForm from "../common/RegisterForm"; poogarniaj absolute path żeby zamiast "../" robić "@/common/RegisterForm", w tym wypadku nie ma to az takiego znaczenia ale jak np bedziesz sie cofal folderowo "../../../../" no to juz wtedy poprawia czytelnosc ;]

  1. Dobrze byłoby zrobić żeby formularz był bardziej generyczny - ale na razie nie jest, więc lepiej zostawić nazwę taką jaka jest. Jeśli się odpowiednio refaktoruje ten komponent tak że ja prawdę będzie generyczny, wtedy się zmieni nazwę.
  2. No to radą na to jest dodać walidację, nie trzeba od razu dodawać kolejnej zależności, i to jeszcze w takim wczesnym stadium.
  3. YAGNI. Jeśli teraz to nie jest problem, to lepiej nie dodawać "na zapas". Problem o którym mówisz występuje jakby showMessage nie było booleanem. Tylko wtedy wystarczy dodać !! zamiast ternary.
  4. Tak
  5. Maybe
  6. Wydzielenia logiki do osobnego pliku jest spoko, ale źle zasugerowałeś kierunek zależności. W Twoim przykładzie logika zależy od widoku - bo jest hookiem. Dobre wydzielenie byłoby takie żeby to widok zależał od logiki. Logika powinna być napisana tak żeby nawet nie wiedziała że jest w aplikacji reaktowej.
  7. Nie ma czegoś takiego jak absolute path w nodzie. Są najwyżej aliasy, ale jak myślisz o nich w kontekście "ścieżka absolutna" to możesz sobie strzelić w stopę.
0
Riddle napisał(a):
RitzCarlton napisał(a):
  1. Zmień nazwe komponentu z RegisterForm na FormInput - bardziej generyczna do zastosowania rowniez w innych formularzach.
  2. Zastanowilbym sie do uzycia jakiegos schema modelu (biblioteka np. Yup) oraz skorzystania z React Hook Form poniewaz brakuje walidacji frontowej.
  3. Nie stosuj takiego zapisu:
{showMessage && (
  <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md">
       <strong>{message}</strong>
   </div>
  )}

gdyż może to powodować potencjalne errory: https://medium.com/geekculture/stop-using-for-conditional-rendering-in-react-a0f7b96200f8
lepiej showMessage ? <div className="mt-4 bg-green-200 text-natural-600 p-2 rounded-md"> <strong>{message}</strong> </div> : null

  1. Powyższy komponent message rowniez wydzielilbym do osobnego komponentu, ogolem warto powydzielac czesci kodu ktore sie powtarzaja do osobnych komponentow (wszystkie Message,Buttony,Inputy itp)
  2. W
const RegisterForm: FC<{
        label: string;
        id: string;
        name: string;
        value: string;
        onChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
        errors: string[] | null;
    }> = ({label, id, name, value, onChange, errors}) => {

stwórz sobie interface np interface IProps {} i przekaz je tak React.FC<IProps> bedzie duzo czytelniejsze.
6. Stworz osobny hook w ktorym bedziesz trzymal logike np usehandleStoreUser lub w tym wypadku w sumie lepiej wydzielic ta funkcje do jakiegos osobnego pliku w stylu registerActions.ts albo registerService.ts i exportowac ja
7. Zamiast import RegisterForm from "../common/RegisterForm"; poogarniaj absolute path żeby zamiast "../" robić "@/common/RegisterForm", w tym wypadku nie ma to az takiego znaczenia ale jak np bedziesz sie cofal folderowo "../../../../" no to juz wtedy poprawia czytelnosc ;]

  1. Dobrze byłoby zrobić żeby formularz był bardziej generyczny - ale na razie nie jest, więc lepiej zostawić nazwę taką jaka jest. Jeśli się odpowiednio refaktoruje ten komponent tak że ja prawdę będzie generyczny, wtedy się zmieni nazwę.
  2. No to radą na to jest dodać walidację, nie trzeba od razu dodawać kolejnej zależności, i to jeszcze w takim wczesnym stadium.
  3. YAGNI. Jeśli teraz to nie jest problem, to lepiej nie dodawać "na zapas". Problem o którym mówisz występuje jakby showMessage nie było booleanem. Tylko wtedy wystarczy dodać !! zamiast ternary.
  4. Tak
  5. Maybe
  6. Wydzielenia logiki do osobnego pliku jest spoko, ale źle zasugerowałeś kierunek zależności. W Twoim przykładzie logika zależy od widoku - bo jest hookiem. Dobre wydzielenie byłoby takie żeby to widok zależał od logiki. Logika powinna być napisana tak żeby nawet nie wiedziała że jest w aplikacji reaktowej.
  7. Nie ma czegoś takiego jak absolute path w nodzie. Są najwyżej aliasy, ale jak myślisz o nich w kontekście "ścieżka absolutna" to możesz sobie strzelić w stopę.
  1. Nazwa RegisterForm sugeruje że jest to jakaś większa część kodu niż sam input, dlatego uważam żeby to zmienić. Możnaby po prostu użyć RegisterFormInput i wtedy jaśniej jest zrozumiane co taki komponent w sobie zawiera.
  2. Racja, ale biorąc pod uwagę że to tylko część kodu i kolega piszę całą apkę (formularzy będzie więcej) warto byłoby zaopatrzyć się w jakąś bibliotekę do nich oraz coś do generowania schemy.
  3. W drugiej części zdania dopisałem że w tym przypadku lepiej będzie wydzielić to do osobnej funkcji, bo faktycznie użycie hooka jest tutaj nie potrzebne
  4. Tak chodziło mi o aliasy, mógłbyś wytłumaczyć dlaczego to zły pomysł? Przy importach jest to dużo czytelniejsze.
0
RitzCarlton napisał(a):
  1. Nazwa RegisterForm sugeruje że jest to jakaś większa część kodu niż sam input, dlatego uważam żeby to zmienić. Możnaby po prostu użyć RegisterFormInput i wtedy jaśniej jest zrozumiane co taki komponent w sobie zawiera.

Nie powinno się nazywać komponentów tym co mają w sobie, tylko tym czym są - po co się ich używa. Autor używa tego komponentu jak formularz, więc powinien się nazywać moim zdaniem tak jak teraz. To co ten komponent renderuje - czyli to ile inputów ma, to już mniej istotne.

Ale tak czy tak - to się odnosi do tego że poleciłeś autorowi zmianę nazwy, żeby komponent był bardziej generyczny. Na co ja odpowiedziałem że sama zmiana nazwy nic nie da i nie jest dobrym pomysłem. Jeśli chciałbyś zmienić komponent, to należałoby go zrefaktorować.

  1. Racja, ale biorąc pod uwagę że to tylko część kodu i kolega piszę całą apkę (formularzy będzie więcej) warto byłoby zaopatrzyć się w jakąś bibliotekę do nich oraz coś do generowania schemy.

Nie koniecznie - dodawanie takich silnych zależności w takim wczesnym stanie to zły pomysł.

Walidację możesz normalnie ogarnąć sam. "generowanie scheme" często przywiązuje aplikacje do biblioteki przez co dużo ciężej ją rozwijać.

Z takimi decyzjami lepiej poczekać, i wprowadzić je później jeżeli w ogóle.

  1. Tak chodziło mi o aliasy, mógłbyś wytłumaczyć dlaczego to zły pomysł? Przy importach jest to dużo czytelniejsze.

Po pierwsze dlatego że dla każdego kto kiedykolwiek pisał w node'zie ścieżki względne są domyślnym sposobem pracy - tak się to po prostu robi.

Wszelkie prace z takimi ścieżkami są ogarniane przez IDE, bo wszystkie IDE ogarniają prace z takimi ścieżkami - czyli na każdym środowisku jakim to odpalisz to będzie dobrze działać, nawet z innymi runtime'ami niż node. Dodatkowo wszystkie toole jak testy, bundlery, litery i każdy tool umie w ścieżki względne - bo to jest naturalny język node'a.

Aliasy to jest dużo późniejszy wynalazek, który powstał w sumie nie wiem czemu - możliwe żeby upodobnić się do innych technologii, ale w mojej ocenie to nie jest zasadne.

Dodatkowo, obsługa takich aliasów zależy od narzędzia który je dodaje, i pomiędzy nimi często są różnice, różnie też działają z takimi zapisami jak np @alias/../something.

Jeśli tego komuś mało, to różne IDE oraz różne toole jak testy muszą być odpowiednio zintegrowane z narzędziem którego używamy do dodania aliasów żeby to działało rzetelnie.

Jako programista, wymagam żeby wszystkie operacje, usuwanie, przenoszenie, kopiowanie, rename, wyszukanie użyć, żeby to wszystko działało dla każdych plików. Jeśli mamy ścieżki względne, to praktycznie każde IDE i każdy tool działa dobrze. Z aliasami zawsze jest jakiś problem, najczęściej z tym że jakiś element systemu ich nie kuma.

Kolejnym elementem jest to, że jak dodajemy katalog wyższego poziomu to musimy pamiętać żeby dodać alias.

No i w końcu, nie widzę zasadności aliasów - jaki jest problem ze ścieżkami relatywnymi? Każde IDE je ogarnia jeśli się z nimi obędziesz, to wszelkie argumenty że to jest "nieczytelne" moim zdaniem są nietrafione. Dla mnie to jest niechęć zmiany nawyków - przy przejściu z technologii gdzie importy są naturalnie względem roota projektu, jak Python czy Java. Ale w node importy są naturalnie względem pliku, i nie ma specjalnej potrzeby żeby to zmienić.

1 użytkowników online, w tym zalogowanych: 0, gości: 1