Feature: Header navigation component #13
Conversation
src/app/Navbar.module.css
Outdated
| .navbar { | ||
| /* style */ | ||
| background: rgba(0, 0, 0, 0.2); | ||
| /* content */ |
There was a problem hiding this comment.
I have a tendency to add comments to my CSS files to organize my CSS properties. Let me know if this is an issue.
There was a problem hiding this comment.
Personal opinion is that it doesn't have much value.
There was a problem hiding this comment.
Minor changes requested. Overall, this looks good and includes a lot of good quality-of-life features (e.g. opening external links in a new tab). I'm not digging too deep into the CSS here either; what's there seems good enough.
Re. Responsiveness, it should generally be considered a core feature due to how often sites are accessed via mobile devices (especially in our case where our website is a quick "go here for info" type of page). In this case, the image problem doesn't really manifest until very low widths; viewing this at standard phone resolutions looks fine. Taking everything into account; let's get this core functionality merged so we can unblock ourselves and circle back to responsiveness later.
src/app/Navbar.module.css
Outdated
| .navbar { | ||
| /* style */ | ||
| background: rgba(0, 0, 0, 0.2); | ||
| /* content */ |
There was a problem hiding this comment.
Personal opinion is that it doesn't have much value.
| <a | ||
| key={link.name} | ||
| target="_blank" | ||
| rel="noopener noreferrer" |
There was a problem hiding this comment.
[Comment]: Good inclusions for opening these links in a new tab - kudos.
| rel="noopener noreferrer" | ||
| href={link.url} | ||
| > | ||
| <Image src={link.icon} alt={`${link.name} Logo`} width="32" height="32" /> |
There was a problem hiding this comment.
[Question]: If width/height are set here, why is it autoscaling the image for mobile views?
There was a problem hiding this comment.
In the file .next/static/css/app/layout.css exists the following selector:
/*
Constrain images and videos to the parent width and preserve their intrinsic aspect ratio. (https://github.com/mozdevs/cssremedy/issues/14)
*/
img,
video {
max-width: 100%;
height: auto;
}This, as stated in the comment, causes the img elements by default to shrink when the container size shrinks.
|
Okay, I made this last change. Are we good to merge? |
Addresses #5. Added Header navigation component named
Navbarin thesrc/appdirectory along withNavbar.module.cssin the same directory. Also added several .png and .svg files in thepublicdirectory for all the logos. Links can be added or removed by editing the array declared within the component.Some things to note: logos are all white-colored. Not sure how this might work if we implement a light theme. Also, this design is not responsive for small screens. As seen in the third image below, the icons shrink down to nothing when the screen becomes too narrow. Whether this should be addressed in this issue or in a separate issue should be decided upon.