-
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: About page (title, description, and image) #12
Conversation
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.
Requesting changes for some style cleanup; rest are comments/suggestions. Overall a good start with this; CSS is just super annoying. One further recommendation - include a screenshot in the PR for any visual changes which helps with easy review.
EDIT: Need to fix the lint check as well.
src/app/about/About.module.css
Outdated
text-align: center; | ||
} | ||
|
||
.about__image_container { |
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.
[Nit]: As this is in About.module.css
, there's no need to use an about__
prefix. Also, prefer kebab-case
for HTML/CSS identifiers (id
/class
) as with URLs (not fully sure the origin for this though).
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.
CSS classNames have been changed to use camelCase as recommended by documentation: https://github.com/css-modules/css-modules#naming
|
||
.about__description { | ||
margin: 0 auto; | ||
max-width: 1000px; |
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]: I see the idea here. Normally, you would have some type of container
class that would handle margin/padding for the page rather than apply it to specific elements (there's a few other tricks too for this to work better with mobile too). Anyways, keep this for now and we'll figure out a more general solution for all pages later as part of global styles.
src/app/about/page.tsx
Outdated
</p> | ||
</div> | ||
|
||
<div className={styles.about__image_container}><Image src="group_photo.jpg" alt="A group of club members sitting in a room looking at the camera and posing" height={600} width={600} /> |
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.
[Suggestion]: We can probably get a bit better with the name/alt - I happen to know this photo is from the Fall 2022 planning meeting. Suggestion:
fall_2022_planning_meeting.jpg
SSD members (about 30) at our Fall 2022 planning meeting in CISE.
I think this better represents the meaning of the photo rather than just being a description, but open to suggestions. Mainly looking ahead to when we (hopefully) have a gallery of images and need to do a better job sorting through that.
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.
@WillBAnders The alt text has since been updated. However, the name of the file is "group_photo.jpg", not "fall_2022_planning_meeting.jpg". Will this discrepancy be 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.
No, I would just recommend the more explicit name to help if/when we add a gallery with multiple pictures where it becomes more important to distinguish each one. But the file can always be renamed at the time; it's more of a callout to prefer more detailed names when possible.
Noticed an issue regarding Git and Prettier. Prettier seems to want line endings to be LF while Git wants line endings to be CRLF. Please clarify what should be used and how to proceed with this PR. |
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 quick change and then good to go.
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.
No issues, as far as I can see.
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 work @Phantom0110 @Brian-Magnuson!
Rough draft of about page. Contains About title with description of club and a picture of the group all centralized on the screen.
resolving #4