Feature: About page (title, description, and image) #12
Conversation
Phantom0110
requested review from
Brian-Magnuson,
WillBAnders,
JavierM333,
yonasbahre and
hohelen
Phantom0110
changed the title
There was a problem hiding this comment.
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.
[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.
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.
[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.
[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.
@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.
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. |
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