Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

14.1 Login-sivun lisääminen Tanstackiin | Antti/add-tanstack-library-login-page #58

Merged
merged 25 commits into from
Jan 17, 2025

Conversation

anttiasmala
Copy link
Collaborator

@anttiasmala anttiasmala commented Nov 6, 2024

Login-sivuun lisätty Tanstack

Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lahjalista ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 11:26am

@anttiasmala anttiasmala changed the title Antti/add-tanstack-library login page 14. Login-sivun lisääminen Tanstackiin | Antti/add-tanstack-library-login-page Nov 6, 2024
pages/login.tsx Outdated Show resolved Hide resolved
pages/login.tsx Outdated Show resolved Hide resolved
utils/apiRequests.ts Outdated Show resolved Hide resolved
utils/apiRequests.ts Outdated Show resolved Hide resolved
@anttiasmala
Copy link
Collaborator Author

anttiasmala commented Nov 6, 2024

TÄHÄN VASTATTU, TÄMÄ OK

Tämä saatta olla jo viides kerta kun kysyn branchien mergeämisestä / siihen liittyvästä tavasta, pahoittelut siitä.

Ajattelin tehdä nyt niin, että teen jokaisen sivun yksi kerrallaan omassa branchissa ja mergeän ne sitten tähän niin sanottuun "pääbranchiin". Huomasin, että helpottaa tosi paljon omaa hommaa, kun on monta "pientä" taskia vs yksi "jättitaski" 😄

Jatkanko tällä tavalla (Tanstack-taskin kohdalla pelkästään), että teen monta PR:ää mutta tosi pieniä niistä. Vai että mergeän omalla koneella kaikki pikkubranchit yhteen "pääbranchiin" (esim. antti/add-tanstack-library) ja teen sitten vain yhden PR:n?

Laitoin myöskin tämän branchin (antti/add-tanstack-library-login-page) menemään antti/add-tanstack-library-branchiin enkä main-branchiin. Ajattelin että mergeää sitten antti/add-tanstack-libraryn main-branchiin kun kaikki sivut on saatu tehtyä. Eli ns valmis branch

@anttiasmala anttiasmala changed the title 14. Login-sivun lisääminen Tanstackiin | Antti/add-tanstack-library-login-page 14.1 Login-sivun lisääminen Tanstackiin | Antti/add-tanstack-library-login-page Nov 7, 2024
@anttiasmala anttiasmala changed the base branch from antti/add-tanstack-library to main December 29, 2024 22:11
pages/login.tsx Outdated
Comment on lines 162 to 173
{isPending ? (
<Button
type="submit"
className="cursor-not-allowed bg-red-500"
disabled
>
Kirjaudutaan sisään
<span className="loading-dots absolute" />
</Button>
) : (
<Button type="submit">Kirjaudu sisään</Button>
)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tein tämmöisen pienen if/else-renderöinnin.

Jos kirjautumisyritys on tehty, tehdään seuraavat toimenpiteet:

  1. estetään napin painaminen
  2. vaihdetaan väri (ei välttämättä hyvä idea, mutta olkoon tässä alussa nyt näin)
  3. laitetaan kursoriksi not-allowed, jotta käyttäjä tietää miksi klikkausta ei rekistöröidä (en tiedä onko tämä hyvä tapa)

Vaihdoin tekstiksi Kirjaudutaan sisään ja lisäsin spanin, jolla asetetaan "pistelatausindikaattori".

Syy absolute-classNamen käyttöön on se, että ilman sitä Kirjaudutaan sisään -teksti liikkui edestakaisin pisteiden mukaan. Se ei näyttänyt hyvältä 😅

Jos ei ole kirjauduttu niin sitten ihan tavallinen kirjautumisnappula näytetään 👍

Onko conditional rendering tässä tapauksessa ok vai olisiko hyvä tehdä esimerkiksi sisäinen komponentti kirjautumisnappulasta?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional rendering on ihan ok, mutta yleensä ei renderöidä näissä tilanteissa kahta eri nappia vaan muutetaan propseja. Punainen ei myöskään ole hyvä väri, vaan disabloitu on yleensä harmaa. Lisäksi yleensä napin tekstiä ei muuteta, vaan nappi disabloidaan, muutetaan harmaaksi ja lisätään joku latausindikaattori (kolme pistettä, spinneri tms.)

Eli voisit tehdä esim. näin:

<Button type="submit" disabled={isLoading}>
  Kirjaudu sisään {isLoading && <span className="loading-dots absolute" />}
</Button>

Sitten disablointityyliä ei kannata määrittää tässä, vaan Button-komponentissa, eli sinne lisäät classNameen disabled:bg-lighterGray disabled:text-middleGray missä korvaat lighterGray ja middleGray sopivilla disablointi-väreillä.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tämä on hyvä, kiitos! 👍

Tästä Tailwindin disabled:iin en ollut törmännyt aiemmin. Se on näppärä 👍

Värien kanssa en ole ihan 100 varma että ovatko hyvät. Kommentoin sen kun oon pushannut 👍

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hyvältä näyttää!

pages/login.tsx Outdated Show resolved Hide resolved
pages/login.tsx Outdated Show resolved Hide resolved
Comment on lines -206 to +213
{isEditModalOpen && editModalGiftData && (
{editModalGiftData && isEditModalOpen && (
<EditModal
gift={editModalGiftData}
setIsModalOpen={setIsEditModalOpen}
/>
)}

{isDeleteModalOpen && deleteModalGiftData && (
{deleteModalGiftData && isDeleteModalOpen && (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nämä kaksi muutosta liittyy ideaan, jossa poistetaan isDeleteModalOpen ja isEditModalOpen. Tieto siitä pitäisikö Modalin olla auki saadaan tarkistamalla sisältääkö esimerkiksi deleteModalGiftData tietoja vai ei

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jep tarkistus voidaan hyvin tehdä noin, mikä onkin järkevää ja yksinkertaistaa asioita. Muuttujien järjestyksen muuttaminen ei taida sitä kuitenkaan vielä tehdä?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joo, ei toimi vielä. Voisin tehdä muokkauksen siihen branchiin, missä lisään Tanstackin EditModal ja DeleteModalille.

Vai onko tämä tarkistus parempi lisätä vasta Tanstackin jälkeen?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voit tehdä siinä samassa branchissä missä lisäät Tanstackin modaaliin.

Copy link
Owner

@samuliasmala samuliasmala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hyvin toimi jo nyt!

pages/login.tsx Outdated
Comment on lines 162 to 173
{isPending ? (
<Button
type="submit"
className="cursor-not-allowed bg-red-500"
disabled
>
Kirjaudutaan sisään
<span className="loading-dots absolute" />
</Button>
) : (
<Button type="submit">Kirjaudu sisään</Button>
)}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional rendering on ihan ok, mutta yleensä ei renderöidä näissä tilanteissa kahta eri nappia vaan muutetaan propseja. Punainen ei myöskään ole hyvä väri, vaan disabloitu on yleensä harmaa. Lisäksi yleensä napin tekstiä ei muuteta, vaan nappi disabloidaan, muutetaan harmaaksi ja lisätään joku latausindikaattori (kolme pistettä, spinneri tms.)

Eli voisit tehdä esim. näin:

<Button type="submit" disabled={isLoading}>
  Kirjaudu sisään {isLoading && <span className="loading-dots absolute" />}
</Button>

Sitten disablointityyliä ei kannata määrittää tässä, vaan Button-komponentissa, eli sinne lisäät classNameen disabled:bg-lighterGray disabled:text-middleGray missä korvaat lighterGray ja middleGray sopivilla disablointi-väreillä.

components/DeleteModal.tsx Outdated Show resolved Hide resolved
Comment on lines -206 to +213
{isEditModalOpen && editModalGiftData && (
{editModalGiftData && isEditModalOpen && (
<EditModal
gift={editModalGiftData}
setIsModalOpen={setIsEditModalOpen}
/>
)}

{isDeleteModalOpen && deleteModalGiftData && (
{deleteModalGiftData && isDeleteModalOpen && (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jep tarkistus voidaan hyvin tehdä noin, mikä onkin järkevää ja yksinkertaistaa asioita. Muuttujien järjestyksen muuttaminen ei taida sitä kuitenkaan vielä tehdä?

shared/types.ts Show resolved Hide resolved
pages/login.tsx Outdated Show resolved Hide resolved
pages/login.tsx Outdated Show resolved Hide resolved
pages/login.tsx Outdated Show resolved Hide resolved
`mt-6 w-full rounded-md border border-lines bg-primary p-2 text-lg font-medium text-white`,
`mt-6 w-full rounded-md border border-lines bg-primary p-2 text-lg font-medium text-white disabled:bg-gray-300 disabled:text-gray-500`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ovatko nuo värit yhtään sinnepäinkään mitä niiden kuuluisi olla? 😅

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oikein hyvät! 👍

Comment on lines +51 to +58
const { mutateAsync, isPending, error } = useMutation({
mutationKey: QueryKeys.LOGIN,
mutationFn: async (loginCredentials: UserLoginDetails) =>
await axios.post('/api/auth/login', loginCredentials),
onSuccess: () => router.push('/'),
});

useShowErrorToast(error);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutationFn saa nyt tarvittavat tiedot parametreina. Lisäsin näemmä myös useShowErrorToast(error) käsittelemään mahdolliset virheet

Copy link
Owner

@samuliasmala samuliasmala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oikein hyvä! 👍 Valmis mergettäväksi.

@anttiasmala anttiasmala merged commit fc3afed into main Jan 17, 2025
5 checks passed
@anttiasmala anttiasmala deleted the antti/add-tanstack-library-login-page branch January 17, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants