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

Antti/modal #2

Merged
merged 46 commits into from
Dec 20, 2023
Merged

Antti/modal #2

merged 46 commits into from
Dec 20, 2023

Conversation

anttiasmala
Copy link
Collaborator

Ensimmäinen prototyyppi modalista tehty.

Antti Asmala added 20 commits October 19, 2023 13:37
…el buttons changes color when mouse is on the element
…nged DeleteModal to appear only once when opened in index.tsxinstead of number of gifts
@vercel
Copy link

vercel bot commented Oct 25, 2023

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 Dec 19, 2023 8:30am

@anttiasmala
Copy link
Collaborator Author

Näköjään tuo TODO.md tuli mukana, vaikka laitoin sen .gitignore-tiedostoon. Eipä siinä mitään salaista ole 😄

Committin tekstejä ei taida pystyä enää jälkikäteen muuttamaan? Huomasin, että olin huonosti laittanut tuon yhden commitin "Changed AcceptDeclineModal to DeleteModal". Kysessä oli .tsx-tiedoston nimen vaihtaminen, tuosta ainakin itse saan käsityksen, että ainoastaan komponentin nimi vaihtui

@samuliasmala
Copy link
Owner

samuliasmala commented Nov 19, 2023

Näköjään tuo TODO.md tuli mukana, vaikka laitoin sen .gitignore-tiedostoon. Eipä siinä mitään salaista ole 😄

Luultavasti lisäsit sen committiin mukaan ennen kuin muokkasit .gitignorea.

Committin tekstejä ei taida pystyä enää jälkikäteen muuttamaan? Huomasin, että olin huonosti laittanut tuon yhden commitin "Changed AcceptDeclineModal to DeleteModal". Kysessä oli .tsx-tiedoston nimen vaihtaminen, tuosta ainakin itse saan käsityksen, että ainoastaan komponentin nimi vaihtui

git rebase ja git amend komennoilla commitin tekstiä voi muuttaa, mutta em. komennon käyttö korvaa muutettavan commitin ja kaikki sen jälkeen tulevat commitit uusilla, joten jos commitit on jo pushattu repoon pitää ne force pushata sinne. Ja jos joku on jo ehtinyt pullaamaan ne, niin tulee konflikteja --> em. komentoja on syytä käyttää vain committeihin, joita ei ole vielä pushannut.

@samuliasmala
Copy link
Owner

Olisiko järkevämpää heti alusta asti tehdä esimerkiksi tästä DeleteModal:sta "globaali", elikkä että sitä voisi käyttä muissa tilanteissa tässä projektissa myös?

Toistaiseksi ei. Sitten jos myöhemmin tulee tarve toiselle vastaavalle, niin voi miettiä siirtääkö joitakin osia DeleteModal:ista yhteiseen ConfirmModal tms. komponenttiin. Mutta yleensä ei kannata kovin paljon tehdä etukäteen vaan vasta kun tarve tulee.

Tämä components/DeleteModal.tsx on toimiva vain tässä lahjaidean poistamisessa eikä se toimi missään muussa tilanteessa.

Onko tämä ihan okei, vai pitäisikö tuosta tehdä "globaali" vaihtamalla esim gift-proppi --> data jne?
Nykyinen on ok.

Jos tuo on ihan okei, että DeleteModal.tsx toimii vain lahjaidean poistamisessa, pitäisikö sille tehdä oma folderi tuonne components-kansion sisään? Esim: components/lahjaidea/DeleteModal.tsx tai muu vastaava, jotta tietää missä DeleteModal.tsx on käytössä / missä se toimii pelkästään?

Ei kannata, nykyinen hakemistorakenne on hyvä.

Esimerkiksi tuo components/Modal.tsx on "globaali" jo nyt, sillä data tulee children-propsina ja kahdelle containerille voi antaa tyylit tarvittaessa className-propsina.

Hmm, onkohan sulla jäänyt pushaamatta jotain? Ainakaan repossa olevassa versiossa tyylejä ei voi antaa className propin avulla.

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.

Alla joitakin kommentteja. Tuo antti/modal_github-action-error-fix minkä meinasit toisessa PR:ssä mergetä lokaalisti taitaa olla vielä pushaama repoon?

{dataToDeleteInfo}
</p>
<AcceptButtonSVG
alt="acceptButton"
Copy link
Owner

Choose a reason for hiding this comment

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

alt-tekstin on tarkoitus korvata kuva jos kuvaa ei jostain syystä voi näyttää (nettiongelma, näkövammaiset), joten Parempi teksti tähän olisi ihmisen luettavaksi tehty esim. "Accept deletion".

}
/>
<DeclineButtonSVG
alt="declineButton"
Copy link
Owner

Choose a reason for hiding this comment

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

Tämä teksti voisi olla esim "Cancel"

@anttiasmala
Copy link
Collaborator Author

Hmm, onkohan sulla jäänyt pushaamatta jotain? Ainakaan repossa olevassa versiossa tyylejä ei voi antaa className propin avulla.

Oli jäänyt pushaamatta ja tuo versio, jossa propeilla pystyy antamaan className:lle arvoja onkin tehty tuonne antti/json-server -branchiin. Katson tuon vaikka Merge-vaiheessa, että onko tarpeellinen, luulen että ei.

Comment on lines +3 to +6
type AcceptButtonType = SVGProps<SVGSVGElement> & {
backgroundFill?: string;
checkmarkFill?: string;
};
Copy link
Owner

Choose a reason for hiding this comment

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

Nyt svgr:n generoimaa koodia on muokattu lisäämällä custom-propit. Sikäli huono, että aina kun ajaa npm run svgr nuo häviävät ja pitää olla varovainen ettei niitä commitoi. @aleksiasmala olisiko sulla joku idea miten tämä kannattaisi tehdä?

Copy link
Owner

Choose a reason for hiding this comment

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

Tätä pitää vielä miettiä voisiko jotenkin parantaa, mutta tehdään se myöhemmin.

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.

Nyt näyttää erinomaiselta! Nämä viimeisimmät muutokset selkeyttivät koodia merkittävästi! 👍

@anttiasmala anttiasmala merged commit d41111d into main Dec 20, 2023
4 checks passed
@anttiasmala anttiasmala deleted the antti/modal branch December 20, 2023 12:50
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.

3 participants