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

Modal component #111

Open
wants to merge 4 commits into
base: frontend
Choose a base branch
from
Open

Modal component #111

wants to merge 4 commits into from

Conversation

A-Wodnicki
Copy link
Member

Zrzut ekranu z 2023-08-04 o 01 23 57-fullpage

Zrzut ekranu z 2023-08-04 o 01 23 43-fullpage

Zrzut ekranu z 2023-08-04 o 01 23 47-fullpage

Zrzut ekranu z 2023-08-04 o 01 48 55-fullpage

@A-Wodnicki A-Wodnicki added frontend Frontend tasks components Create new component labels Aug 3, 2023
@A-Wodnicki A-Wodnicki self-assigned this Aug 3, 2023
@A-Wodnicki A-Wodnicki linked an issue Aug 3, 2023 that may be closed by this pull request
@A-Wodnicki A-Wodnicki marked this pull request as ready for review August 22, 2023 14:22
Copy link
Collaborator

@marcol13 marcol13 left a comment

Choose a reason for hiding this comment

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

Zapisuję tyle ile zdążyłem przejrzeć na szybko 😅 Komponent jeszcze do przedyskutowania 😉 Ale ogólnie gratulacje za solidnie przygotowany kod i ciekawe rozwiązania!

* - [dialog](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog)
* */
function Modal({ title, children, modal, variant, open, onClose }) {
let showConfirm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Możesz tutaj użyć ternary operator i usunąć tego leta. Chyba, że masz jakiś głębszy zamysł z użyciem let w tym przypadku 😅

Suggested change
let showConfirm
const showConfirm = variant === "confirm" ? true : false;

Copy link
Member Author

Choose a reason for hiding this comment

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

Zrobiłem tak w razie wypadku gdyby w przyszłości miałoby się pojawić więcej wariantów, niżej jest switch, który by tym potem sterował

}, [open])

return (
<dialog
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nie jestem przekonany czy użycie elementu dialog jest najlepsze, ze względu na to, że nie wszędzie jest to jeszcze wspierane. Przedyskutujmy to na najbliższym spotkaniu 😅

console.log('clicked')
}}
>
try clicking me
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ten button jest chyba niepotrzebny 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Chciałem jakoś pokazać, że można wchodzić w interakcję z resztą strony, kiedy prop modal ma wartość false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
components Create new component frontend Frontend tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Frontend] Komponent: Modal
2 participants