-
Notifications
You must be signed in to change notification settings - Fork 62
Pull request for code review #56
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
base: main
Are you sure you want to change the base?
Conversation
…with mobile first approach
carro-barro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Först vill jag säga riktigt bra jobbat, detta projekt har tror jag för många i vår grupp varit svårt, iallafall för mig. men också väldigt roligt - hoppas du också känner så!
här kommer lite generella kommentarer:
Snyggt jobbat med responsiviteten ser verkligen proffsigt ut, exempelvis hur hero bilden skalas ner på mobil och att texten flyttas ner i två rader, gillar verkligen hur bilderna är staplade på mobil också!
Gillar verkligen segmentet med ”about us” och formuläret strukturen är väldigt clean, tycker att användningen av padding/margin här gör att sidan är balanserad och en fröjd för ögat 😉
tycker i helhet att projektet är välarbetat, har bara lagt till några mindre kod förslag 🤩
index.html
Outdated
| <!---NAVBAR-------------> | ||
| <ul class="nav-list"> | ||
| <li><a href="#">Home </a></li> | ||
| <li><a href="#">Models </a></li> | ||
| <li><a href="#">About us </a></li> | ||
| <li><a href="#">Contact </a></li> | ||
| </ul> | ||
|
|
||
| <!---HAMBURGER MENU--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superbra användning av kommentarer - det ska jag ta med mig till mitt projekt!
index.html
Outdated
| <!---NAVBAR-------------> | ||
| <ul class="nav-list"> | ||
| <li><a href="#">Home </a></li> | ||
| <li><a href="#">Models </a></li> | ||
| <li><a href="#">About us </a></li> | ||
| <li><a href="#">Contact </a></li> | ||
| </ul> | ||
|
|
||
| <!---HAMBURGER MENU--> | ||
| <ul | ||
| id="menu" | ||
| class="menu" | ||
| > | ||
| <span class="hamburger-links"> | ||
| <li class="hamburger-text"><a | ||
| class="menuItem" | ||
| href="#" | ||
| >Home</a></li> | ||
| </span> | ||
| <span class="hamburger-links"> | ||
| <li class="hamburger-text"><a | ||
| class="menuItem" | ||
| href="#" | ||
| >Models</a></li> | ||
| </span> | ||
| <span class="hamburger-links"> | ||
| <li class="hamburger-text"><a | ||
| class="menuItem" | ||
| href="#" | ||
| >About us</a></li> | ||
| </span> | ||
| <span class="hamburger-links"> | ||
| <li class="hamburger-text"><a | ||
| class="menuItem" | ||
| href="#" | ||
| >Contact</a></li> | ||
| </span> | ||
| </ul> | ||
| <button | ||
| id="hamburger" | ||
| class="hamburger" | ||
| > | ||
| <i | ||
| id="menuIcon" | ||
| class="menuIcon material-icons" | ||
| >menu</i> | ||
| <i | ||
| id="closeIcon" | ||
| class="closeIcon material-icons" | ||
| >close</i> | ||
| </button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snyggt jobbat med att skriva bra och beskrivande id/class namn - men var lite nyfiken över att nav-desktop och nav-mobil inte ligger i varsin div? blir nyfiken då jag själv har ganska många div och isåfall kanske kan ta bort några av mina.
index.html
Outdated
| </header> | ||
|
|
||
| <!----MAIN section GRID---------> | ||
| <section class="main-section"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finns det någon anledning till att section tagen används istället för main tagen? skulle gärna vilja höra om hur det funkade för dig.
| </section> | ||
|
|
||
|
|
||
| <!--INFO + FORM section-------> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
igen bra kommentar !!
| <h3>Request a booking with willz</h3> | ||
| <fieldset> | ||
| <label> | ||
| <span class="customer-name">Name</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
här används span - är nyfiken över det, själv använde jag label och lade namnet i den, hade varit kul att veta hur det funkat med span och om det blir någon skillnad beroende på vilken man använder :)
style.css
Outdated
| /* Footer section*/ | ||
| footer { | ||
| background: #658382; | ||
| text-align: center; | ||
| padding: 40px; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gillar verkligen att du lagt till en footer som matchar i färg med din hero bild, ser super snyggt ut och får sidan att se mer ihop hängande ut.
style.css
Outdated
| .menu { | ||
| display: none; | ||
| } | ||
| } | ||
|
|
||
| .showMenu { | ||
| transform: translateY(0); | ||
| } | ||
|
|
||
| .hamburger { | ||
| position: fixed; | ||
| z-index: 100; | ||
| top: 30px; | ||
| right: 2px; | ||
| padding: 15px; | ||
| border: none; | ||
| background: none; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .hamburger .closeIcon { | ||
| display: none; | ||
| } | ||
|
|
||
| .hamburger .menuIcon { | ||
| display: inline-block; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man skulle kunna fixa lite med hamburgar menyn, man skulle kunna göra den lite större och försöka på något sätt göra den lite mer synligt när det ligger över hero bilden, då försvinner den lite eftersom bilden och menyn inte har sån hög kontrast mot varandra. kanske en bakgrundsfärg eller ljusskugga
| .info-section { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| margin-bottom: 100px; | ||
| margin-top: 60px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Möjligtvis att man kan kolla lite på ”about us” texten lite på mobil för att se om man kanske kan dra ut den lite längre i kanterna så att den inte blir så smal
script.js
Outdated
| function toggleMenu() { | ||
| if (menu.classList.contains("showMenu")) { | ||
| menu.classList.remove("showMenu") | ||
| closeIcon.style.display = "none" | ||
| menuIcon.style.display = "block" | ||
| } else { | ||
| menu.classList.add("showMenu") | ||
| closeIcon.style.display = "block" | ||
| menuIcon.style.display = "none" | ||
| } | ||
| } | ||
|
|
||
| hamburger.addEventListener("click", toggleMenu) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
har lite svårt att förstå javascript helt och hållet, så förstår inte helt denna kod - men jag tycker det är intressant att du använt function toggleMenu( )+ resten av koden, då jag själv använt hamMenu.addEventListener("click", () => { hamMenu.classList.toggle("active") hamMenuLinks.classList.toggle("show") })
script.js
Outdated
| const bookingRequest = () => { | ||
| alert("Great choice.We will get back to you shortly with more details".) | ||
| } | ||
|
|
||
| submitButton.addEventListener("click", bookingRequest)*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trr jag förstår vad du försökt göra här med att ge en popup efter submit - kul idé!!, ska nog själv lägga till något liknande på min :)
…d links in navigation bar
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little note: be careful when restyling checkboxes and radio buttons, as users tend to be used to radio buttons being round and checkboxes being square 😇
Beautiful-looking grid and well-working JavaScript. Keep up the good work!
PS. I think it would be nice if the button had the same font as the rest of the page.
| <li><a href="#homePage">Home </a></li> | ||
| <li><a href="#modelSection">Models </a></li> | ||
| <li><a href="#information">About us </a></li> | ||
| <li><a href="#formSection">Contact </a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember indentation
index.html
Outdated
| class="form-section" | ||
| > | ||
| <form | ||
| action="http://httpbin.org/anything" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to use the https method, as some browser will otherwise block this action
style.css
Outdated
| padding: 0; | ||
| top: 0; | ||
| display: flex; | ||
| Padding: 20px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double padding 👀 And remember that all properties are written in lower-case
script.js
Outdated
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| /* OLD JAVASCRIPT | ||
| const menu = document.getElementById("menu") | ||
| const menuItems = document.getElementsByClassName("menuItem") | ||
| const hamburger = document.getElementById("hamburger") | ||
| const closeIcon = document.getElementById("closeIcon") | ||
| const menuIcon = document.getElementById("menuIcon") | ||
|
|
||
| function toggleMenu() { | ||
| if (menu.classList.contains("showMenu")) { | ||
| menu.classList.remove("showMenu") | ||
| closeIcon.style.display = "none" | ||
| menuIcon.style.display = "block" | ||
| } else { | ||
| menu.classList.add("showMenu") | ||
| closeIcon.style.display = "block" | ||
| menuIcon.style.display = "none" | ||
| } | ||
| } | ||
|
|
||
| hamburger.addEventListener("click", toggleMenu)*/ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One empty line is enough 😉 And you can remove unused code
index.html
Outdated
| src="images/willz-dreaming.png" | ||
| alt="cat" | ||
| > | ||
| <Professional>Forget about casting calls, multiple models, or diva drama. Willz is the one-cat wonder your brand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What HTML element is this? 👀
…mmented out, updated readme file
Pull request for code review on project business-site
https://willz-models.netlify.app/