-
Notifications
You must be signed in to change notification settings - Fork 62
Saras project-buisiness-site #60
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
sandrahagevall
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.
Great job with your business site! It looks really good, clean and professional. I think both the HTML and CSS are well-structured and it's easy to follow along with your code. I really like how the images in the portfolio grid subtly pop up on hover—it adds a nice interactive touch. Overall, the whole site looks really calm and inviting. This is absolutely something your photographer-friend could use in real life :)
Once again, good job! You should be really proud of what you've achieved with your site :)
index.html
Outdated
| id="message" | ||
| rows="5" | ||
| cols="50" | ||
| placeholder="Let´s talk-what´s you´re vision?" |
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.
In the textarea placeholder ("Let´s talk-what´s you´re vision?"), you use a backtick instead of apostrophe, could replace it with apostrophe for better readability ("Let's talk – what's your vision?")
style.css
Outdated
| @media (min-width: 600px) { | ||
| .grid { | ||
| grid-template-columns: repeat(3, 1fr); | ||
| } | ||
| } | ||
|
|
||
| @media (min-width: 1024px) { | ||
| .grid { | ||
| grid-template-columns: repeat(3, 1fr); | ||
| } | ||
| } |
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.
You have the same code for media query min-width: 600px and min-width: 1024px, you can remove the media query for min-width: 1024px, since no more changes will happen when reaching 1024px.
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Photography by Louise</title> | ||
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Great+Vibes&family=Lato:wght@400;700&display=swap" rel="stylesheet"> |
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.
I guess you have forgotten to add this font link to the portfolio.html, so when you go to the portfolio-side, the h1 (Photography by Louise) have another font.
style.css
Outdated
| font-weight: bold; | ||
| letter-spacing: 1px; | ||
| position: relative; | ||
| transition: all 0.3 ease; |
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.
Missing a "s" for seconds (all 0.3s ease;)
index.html
Outdated
| alt="wedding-beach" | ||
| /> | ||
| <h3>Wedding</h3> | ||
| <p>Romantic pictures from you´re big day.</p> |
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.
Typo: Should be "Romantic pictures from your big day"
index.html
Outdated
| action="http://httpbin.org/anything" | ||
| method="POST" | ||
| > | ||
| <div class="formt-input"> |
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.
Typo: "formt-input", I guess it should be "form-input". So for now the "Name" textfield in the form doesn't have the same spacing as "type of session", "preffered date" etc.
index.html
Outdated
| alt="Couple-photo" | ||
| /> | ||
| <h3>Couples</h3> | ||
| <p>Capture love and you´re special moments</p> |
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.
Typo: Should be ".. your special moments"
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.
Thank you Sandra:) fixed everything now i think.
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.
Make sure your project looks good from 320px width and up. Right now you have some content that are wider than this which makes it look broken on small phones. Also the hamburger menu is kind of crashing with the heading.
Nice use of flexbox of CSS Grid!
PS. I will approve this as a hero image but for future reference, a hero image usually covers the whole page horizontally (no padding/margin on the sides).
Change request
- Fix responsiveness so that everything fits on mobile
- Make the hamburger menu readable
index.html
Outdated
| > | ||
| <h2>Make a request</h2> | ||
| <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
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.
This is now fixed:)
script.js
Outdated
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
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 😉
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.
got it😉
SaraEnderborg
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.
changes requested are now fixed:)
|
Hi! Not sure if this is where i reply but i have also commented in github and the changes requested are now fixed:)
Thanks for the feedback.
________________________________
Från: Matilda Brunemalm ***@***.***>
Skickat: den 24 september 2025 11:47
Till: Technigo/js-project-business-site ***@***.***>
Kopia: Sara Enderborg ***@***.***>; Author ***@***.***>
Ämne: Re: [Technigo/js-project-business-site] Saras project-buisiness-site (PR #60)
@HIPPIEKICK requested changes on this pull request.
Make sure your project looks good from 320px width and up. Right now you have some content that are wider than this which makes it look broken on small phones. Also the hamburger menu is kind of crashing with the heading.
Nice use of flexbox of CSS Grid!
PS. I will approve this as a hero image but for future reference, a hero image usually covers the whole page horizontally (no padding/margin on the sides).
Change request
* Fix responsiveness so that everything fits on mobile
* Make the hamburger menu readable
________________________________
In index.html<#60 (comment)>:
+ src="../images/kid1.jpg"
+ alt="Family/Children-photo"
+ />
+ <h3>Family/Children</h3>
+ <p>Natural and playful portraits of family/children</p>
+ </article>
+ </section>
+ </main>
+
+ <section
+ id="contact"
+ class="contact"
+ >
+ <h2>Make a request</h2>
+ <form
+ action="http://httpbin.org/anything"
Make sure to use the https method, as some browser will otherwise block this action
________________________________
In script.js<#60 (comment)>:
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
One empty line is enough 😉
—
Reply to this email directly, view it on GitHub<#60 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BWLQHO6ZLL5QM2NETQT7MUL3UJSCDAVCNFSM6AAAAACGYLZ7EKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTENRRHE2TKMZYGU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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.
Great improvements!
Here is the link to my project:)
https://meek-entremet-8e1086.netlify.app/