-
Notifications
You must be signed in to change notification settings - Fork 1
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal opinion is that it doesn't have much value.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question]: If width
/height
are set here, why is it autoscaling the image for mobile views?
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 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.
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 code-quality change that I thought was in the first review; my mistake. Otherwise LGTM.
Okay, I made this last change. Are we good to merge? |
Addresses #5. Added Header navigation component named
Navbar
in thesrc/app
directory along withNavbar.module.css
in the same directory. Also added several .png and .svg files in thepublic
directory 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.