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

scroll Up #52

Merged
merged 6 commits into from
Jan 3, 2024
Merged

scroll Up #52

merged 6 commits into from
Jan 3, 2024

Conversation

patryq03
Copy link
Collaborator

dodalem sccs przycisku i plik js ale nie wiedzialem gdzie dać przycisk, wiec dalem go zakomentowanego w scss...

dodalem sccs przycisku i plik js ale nie wiedzialem gdzie dać przycisk, wiec dalem go zakomentowanego w scss...
Copy link
Owner

@Valik3201 Valik3201 left a comment

Choose a reason for hiding this comment

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

Masz dużo konfliktów. Nie zaktualizowałeś main przed pull requestem

@Valik3201
Copy link
Owner

Valik3201 commented Dec 31, 2023

Moje propozycje:

  1. Zalecam zmianę metody obsługi zdarzeń z użycia atrybutu onclick na bardziej nowoczesny sposób, tj. dodanie event listenera. Zamiast tego:
<button id="scrollUp" onclick="scrollUp()">Scroll Up</button>

Możemy użyć:

<button id="scrollUp">Scroll Up</button>

i w pliku scrollUp.js dodać:

document.getElementById('scrollUp').addEventListener('click', scrollUp);

Dodanie event listenera poprawia separację treści i prezentacji oraz ułatwia zarządzanie zdarzeniami.

  1. Proponuję, aby przycisk "Scroll Up" nie był widoczny od razu, ale po przewinięciu strony. To doskonałe ulepszenie wizualne, które poprawia interfejs.

  2. Poprawiłem plik SCSS, ponieważ został dodany nowy plik o nazwie "scrollUp.scss", który generuje mapę do pliku "scrollUp.css". Ten plik nie był jednak nigdzie podłączony. Zmieniłem nazwę pliku na "_scrollUp.scss" i dodałem import do głównego pliku "main.scss". Następnie skompilowałem wszystkie zmiany.

  3. Zaktualizowałem zależności JavaScript, ponieważ plik "index.js" został zastąpiony jednym wspólnym plikiem o nazwie "common.js" dla obu stron, tak jak pisałem na Slacku.

@Valik3201
Copy link
Owner

Zaktualizowałem obie strony (index.html i shopping-list.html), dodając element .

I proszę, abyś kontynuował stylizację, zgodnie z figmą

@Valik3201 Valik3201 self-requested a review January 3, 2024 15:22
@Valik3201 Valik3201 merged commit a59c468 into main Jan 3, 2024
@Valik3201 Valik3201 deleted the feat/scrollUp branch January 3, 2024 17:35
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