PR: review of business project #22
Conversation
There was a problem hiding this comment.
Hello Christina, overall a very insightful review for me, clean and easy to watch in different documents. Missing some comments but I think that is because I personally dont fully understand everything just yet :). I am not asking for changes, rather explanations so no stress on that. Thanks for a well structured code.
script.js
Outdated
There was a problem hiding this comment.
Great use javascritp, makes the site very interactive and "in motion". A plus for the comments to make it clear what is happening.
There was a problem hiding this comment.
Clean and clear to look at. Like the use of section and division. So section is for content that has a certain topic? And div is more for layout, general?
There was a problem hiding this comment.
Yes that's how I used it! I thought of it as the different parts of my site.
| <nav class="nav"> | ||
| <ul class="nav-links"> | ||
| <li><a href="#top" class="nav-link">Top</a></li> | ||
| <li><a href="#about" class="nav-link">About</a></li> | ||
| <li><a href="#shop" class="nav-link">Shop</a></li> | ||
| <li><a href="#events" class="nav-link">Events</a></li> | ||
| <li><a href="#membership" class="nav-link">Membership</a></li> | ||
| <li> | ||
| <a href="#" class="nav-link nav-btn" | ||
| ><ion-icon name="cart" class="cart-icon"></ion-icon | ||
| ><span class="cart-text">Cart</span></a | ||
| > | ||
| </li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
Would you say it is always important having ul and li in a navbar? I have seen other options.
There was a problem hiding this comment.
I think it's a preference, I find it easier to identify in my code.
| /* UNIVERSAL STYLES */ | ||
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| html { | ||
| scroll-behavior: smooth; | ||
| } | ||
|
|
||
| body { | ||
| font-family: "Roboto", sans-serif; | ||
| background-color: #fdfaff; | ||
| color: #2f184b; | ||
| } | ||
|
|
||
| p { | ||
| line-height: 1.5; | ||
| font-size: 1.2rem; | ||
| } | ||
|
|
||
| /* SECTIONS */ | ||
| .section { | ||
| max-width: 1800px; | ||
| margin: 0 auto; | ||
| padding: 10rem 1rem 10rem 1rem; | ||
| } | ||
|
|
||
| .section-text { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, 1fr); | ||
| gap: 2rem; | ||
| } | ||
|
|
||
| .section-title { | ||
| font-size: 3rem; | ||
| margin-bottom: 3rem; | ||
| } | ||
|
|
||
| .section-subtitle { | ||
| font-size: 1.6rem; | ||
| margin-bottom: 2rem; | ||
| } | ||
|
|
||
| .section-desc { | ||
| margin-bottom: 2rem; | ||
| } | ||
|
|
||
| .dark-bg-section { | ||
| background-color: #eeebf0; | ||
| } | ||
|
|
There was a problem hiding this comment.
You said this in the demo, that since all sections have the same style it is easier to put them here. I understand that. But, if you then wanted to change one section and keep the rest the same, would you do that in your style.css? Otherwise, really like this way of thinking.
There was a problem hiding this comment.
Yep! So if I have any specific changes for certain areas I just do that in the style.css, anything that I use repeatedly across the page I have put in general.css. Again I think its a preference for organising things.
| z-index: 999999; | ||
| } |
There was a problem hiding this comment.
It just puts it above everything else so it's visible :) (I just put a really big number to be certain).
|
Okay, thanks a lot for your time answering my comments. Enjoyed this. |
There was a problem hiding this comment.
Really good job with this project! Especially like your take on the stretch goals. One thing I need you to fix however is that your labels don’t match (e.g. you have two labels called email and one label called movie, but no matching input with that id). Fix this and you’re good to go 💪
And a reminder for coming projects: Remember to indent attributesif you have two or more of them, it will make the code easier to read.
Changes requested
- Proper labels
|
Thanks for the feedback! I will work on correcting the labels, I must have just copied and not changed them accordingly, sorry! I have a question about indenting attributes though, I have prettier installed on my vscode so it doesn't allow me to indent things myself really (unless I misunderstood what you meant), if I indent it just reverts them back, it only seems to indent when the number of attributes gets longer than a certain amount, I'd rather not remove prettier, is there another way around this or is this ok? |
|
You should be able to configure your preferences for Prettier, yes!
Prettier is a tool, but you set the rules :D I'd probably look into their
docs <https://prettier.io/>.
…On Wed, Mar 5, 2025 at 9:47 AM Christina Baldwin ***@***.***> wrote:
Thanks for the feedback! I will work on correcting the labels, I must have
just copied and not changed them accordingly, sorry! I have a question
about indenting attributes though, I have prettier installed on my vscode
so it doesn't allow me to indent things myself really (unless I
misunderstood what you meant), if I indent it just reverts them back, it
only seems to indent when the number of attributes gets longer than a
certain amount, I'd rather not remove prettier, is there another way around
this or is this ok?
—
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMJEL4L47U622I5SNOLHNMT2S22Y3AVCNFSM6AAAAABXQRFRZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBQGI2DOMRUGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: christina-baldwin]*christina-baldwin* left a comment
(Technigo/js-project-business-site#22)
<#22 (comment)>
Thanks for the feedback! I will work on correcting the labels, I must have
just copied and not changed them accordingly, sorry! I have a question
about indenting attributes though, I have prettier installed on my vscode
so it doesn't allow me to indent things myself really (unless I
misunderstood what you meant), if I indent it just reverts them back, it
only seems to indent when the number of attributes gets longer than a
certain amount, I'd rather not remove prettier, is there another way around
this or is this ok?
—
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMJEL4L47U622I5SNOLHNMT2S22Y3AVCNFSM6AAAAABXQRFRZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBQGI2DOMRUGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Matilda Brunemalm
Code coach & frontend developer
Technigo
***@***.*** | www.technigo.io
[image: Facebook icon] <https://www.facebook.com/heytechnigo/> [image:
LinkedIn icon] <https://www.linkedin.com/school/22312818/admin/> [image:
Instagram icon] <https://www.instagram.com/heytechnigo/>
|
|
Great! I'll look into it, thanks! |
No description provided.