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

Toast-ilmoitusten ajan nollaaminen | Antti/toast reset timer on hover #43

Merged
merged 14 commits into from
Oct 29, 2024

Conversation

anttiasmala
Copy link
Collaborator

Toastien ajan nollaamisen toteutus. Näin tuon ominaisuuden Bitwardenin toasteissa. Tykkäsin ideasta, joten halusin kokeilla lisätä itsekin sen.

react-toastify ei näyttäisi tukevan onHover-eventtiä, joten toteutin "itse". En löytänyt mitään ohjeita / toteutustapaa netistä tähän liittyen, joten tämä luultavasti on aika hirveän näköinen mutta katsotaan 🤣

Maininta kommentissa: #35 (comment)

Copy link

vercel bot commented Oct 17, 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 Oct 26, 2024 2:53pm

utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
Comment on lines 21 to 23
if (!document) {
return;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Onko tämä tarpeellinen tsekkaus?

Puhuttiin silloin serverin puolen rendauksesta, eikö se voi tapahtua tätä funktiota kutsuessa? Silloinhan document ei pitäisi olla olemassa.

Jos tämä tsekkaus säilytetään, vaihtaisin paikkaa rivin 20 generatedToastID:n kanssa, jotta turhaan ei generoi ID:tä, jos sitä ei voidakaan käyttää. Käytännössä lienee turha muokkaus

Copy link
Owner

Choose a reason for hiding this comment

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

Tsekkaus ei ole tarpeellinen, koska handleErrorToast funktiota ei koskaan kutsuta serverillä.

Copy link
Collaborator Author

@anttiasmala anttiasmala Oct 23, 2024

Choose a reason for hiding this comment

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

Varmistan vielä että ymmärsin oikein. Eli jos handleErrorToast kutsuttaisiinkin backendissä, silloin tarkistus on tarpeellinen?

Toinen kysymys vielä joka ei tähän varsinaisesti liity. Mutta useEffect-funktiota .tsx-päätteisissä tiedostoissa ei ajeta serverillä koskaan, jonka takia esimerkiksi seuraavia tsekkauksia ei tarvitse tehdä:

useEffect(() => {
   if(window){
   // code   
}
  }, []);

tai

useEffect(() => {
   if(document){
   // code   
}
  }, []);

window ja document ovat vain client-puolen funktioita / classeja / objekteja / tai millä nimellä kutsutaankaan

Copy link
Owner

Choose a reason for hiding this comment

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

Varmistan vielä että ymmärsin oikein. Eli jos handleErrorToast kutsuttaisiinkin backendissä, silloin tarkistus on tarpeellinen?

Kyllä, juurikin näin.

Toinen kysymys vielä joka ei tähän varsinaisesti liity. Mutta useEffect-funktiota .tsx-päätteisissä tiedostoissa ei ajeta serverillä koskaan, jonka takia esimerkiksi seuraavia tsekkauksia ei tarvitse tehdä:

useEffect(() => {
   if(window){
   // code   
}
  }, []);

tai

useEffect(() => {
   if(document){
   // code   
}
  }, []);

window ja document ovat vain client-puolen funktioita / classeja / objekteja / tai millä nimellä kutsutaankaan

Jep window ja document ovat objekteja jotka on määritetty cain client-puolella. Mutta koska useEffect kuten myös kaikki onClick handlerit ajetaan vain client-puolella niissä tarkistusta ei tarvitse tehdä.

utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
Comment on lines 21 to 23
if (!document) {
return;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Tsekkaus ei ole tarpeellinen, koska handleErrorToast funktiota ei koskaan kutsuta serverillä.

utils/handleToasts.ts Outdated Show resolved Hide resolved
@samuliasmala
Copy link
Owner

@anttiasmala Mun ehdotukset window.crypto.randomUUID:n käyttämiseksi ja handleErrorToast` muuttamiseksi olivat vain huomioita. Sun toteutus oli hyvä ja toimii joten ei tarvitse tehdä mitään muutoksia.

@anttiasmala
Copy link
Collaborator Author

anttiasmala commented Oct 23, 2024

@anttiasmala Mun ehdotukset window.crypto.randomUUID:n käyttämiseksi ja handleErrorToast` muuttamiseksi olivat vain huomioita. Sun toteutus oli hyvä ja toimii joten ei tarvitse tehdä mitään muutoksia.

Mun mielestä sun idea oli parempi! 👍

Jätin kuitenkin Toastien ID:n käyttämään meidän omaa tapaa (window.crypto.randomUUID()), koska se tuntuu hiukan paremmalta omaan makuun kuin mikä Toasteissa oletuksena on

Mergesin samalla main-branchin tähän 👍

utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
utils/handleToasts.ts Outdated Show resolved Hide resolved
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 toimii! 👍

@anttiasmala
Copy link
Collaborator Author

Valmista, mergetään!

@anttiasmala anttiasmala reopened this Oct 29, 2024
@anttiasmala anttiasmala merged commit 59082b2 into main Oct 29, 2024
5 of 6 checks passed
@anttiasmala anttiasmala deleted the antti/toast-reset-timer-on-hover branch October 29, 2024 09:04
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