Add Sections Component #49

Merged
j285he merged 12 commits from j285he-sections into main 3 months ago
Collaborator
Closes #43. Will look better when !46 is merged in. https://j285he-sections-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/
j285he added 2 commits 3 months ago
d0dc94441d Initial sections commit
ff2ec0ce3b Add Sections
j285he requested review from a258wang 3 months ago
j285he requested review from snedadah 3 months ago
j285he requested review from e26chiu 3 months ago
j285he requested review from b72zhou 3 months ago
snedadah reviewed 3 months ago
export default function Sections({ data }: SectionsProps) {
return (
<section className={styles.sections}>
<h1>Sections</h1>
Owner

It would be super useful if we add a prop like showHeader, and if it is false, it will not show the header or the separator. That way we will be able to reuse this component for the nav bar, which has the exact same list but a different header.

It would be super useful if we add a prop like `showHeader`, and if it is false, it will not show the header or the separator. That way we will be able to reuse this component for the nav bar, which has the exact same list but a different header.
j285he marked this conversation as resolved
{data.map((datum, index) => {
return (
<div key={`${datum.name}-${index}`} className={styles.navItem}>
<h4>{String(index).padStart(2, "0")}</h4>
Owner

I wonder if we should move the number inside the a tag below so it's also clickable?

I wonder if we should move the number inside the a tag below so it's also clickable?
Owner

I think having the numbers be clickable might be nice!

I think having the numbers be clickable might be nice!
j285he marked this conversation as resolved
snedadah reviewed 3 months ago
data: SectionsData[];
}
export default function Sections({ data }: SectionsProps) {
Owner

We should change this to a non-default export "export function Sections" since thats the convention we are following.

We should change this to a non-default export "export function Sections" since thats the convention we are following.
j285he marked this conversation as resolved
a258wang reviewed 3 months ago
.sections {
display: flex;
flex-direction: row;
width: 50%;
Owner

This might not look great on smaller screens - maybe just try setting a reasonable max-width?

This might not look great on smaller screens - maybe just try setting a reasonable `max-width`?
Poster
Collaborator

did you mean min-width? I tried out max-width but it did not look too good on smaller screens.

did you mean `min-width`? I tried out max-width but it did not look too good on smaller screens.
Owner

Hmm, I was playing around with the browser dev tools, and maybe we don't even need to set the min/max width at all and it'll still look the same?

I think we can just style the component separately for mobile screens, but that can wait for a later date.

Hmm, I was playing around with the browser dev tools, and maybe we don't even need to set the min/max width at all and it'll still look the same? I think we can just style the component separately for mobile screens, but that can wait for a later date.
display: flex;
flex-direction: row;
width: 50%;
gap: 15px;
Owner

NIT: calc(15rem / 16)

NIT: `calc(15rem / 16)`
j285he marked this conversation as resolved
display: flex;
flex-direction: row;
align-items: flex-start;
margin: calc(20rem / 16);
Owner

There's appears to be a larger space between the "Sections" header and the separator vs. the nav items and the separator. Adding margin-left: 0 should be one way to make the spaces equally sized.

There's appears to be a larger space between the "Sections" header and the separator vs. the nav items and the separator. Adding `margin-left: 0` should be one way to make the spaces equally sized.
j285he marked this conversation as resolved
}
.nav a:hover {
color: var(--primary-accent-light);
Owner

NIT: I personally am not a big fan of the colour change on hover - I think the underline and pointer cursor are sufficient hover effects. Up to you though. :)

(Getting rid of the colour change would also be helpful if we make the numbers clickable as well.)

NIT: I personally am not a big fan of the colour change on hover - I think the underline and pointer cursor are sufficient hover effects. Up to you though. :) (Getting rid of the colour change would also be helpful if we make the numbers clickable as well.)
j285he marked this conversation as resolved
export const sectionsData = [
Owner

NIT: maybe name this file sections.ts? I guess routes.ts also makes sense though.

NIT: maybe name this file `sections.ts`? I guess `routes.ts` also makes sense though.
Poster
Collaborator

Was thinking that the bottom nav component might need a list of all pages so we could just use this as a single source of truth

Was thinking that the bottom nav component might need a list of all pages so we could just use this as a single source of truth
a258wang marked this conversation as resolved
j285he added 1 commit 3 months ago
5c87e7636e Code review fixes
j285he added 2 commits 3 months ago
7f64de74b7 Merge main
Poster
Collaborator

Note: After moving the component out of the playground's <h2> tag (which was a mistake), I think the alignment of the separator, the middle of the h1, and the middle of the first anchor is slightly unaligned. i will change it when the text fixes are done

Note: After moving the component out of the playground's `<h2>` tag (which was a mistake), I think the alignment of the separator, the middle of the h1, and the middle of the first anchor is slightly unaligned. i will change it when the text fixes are done
j285he added 1 commit 3 months ago
c9522ece4b Remove margin: 0 auto
a258wang approved these changes 3 months ago
a258wang left a comment
Owner

Added some more small comments. I'm going to wait until after #46 is merged and the padding is fixed to see what this looks like before approving.

Added some more small comments. I'm going to wait until after #46 is merged and the padding is fixed to see what this looks like before approving.
.sections {
display: flex;
flex-direction: row;
min-width: calc(1000rem / 16);
Owner

Continuing from thoughts here: #49

Maybe try getting rid of min-width and see what that looks like? I suspect it'll be fine if we don't specify the width at all.

Continuing from thoughts here: https://git.csclub.uwaterloo.ca/www/cs-2022-class-profile/pulls/49#issuecomment-11099 Maybe try getting rid of min-width and see what that looks like? I suspect it'll be fine if we don't specify the width at all.
Poster
Collaborator

Removed min-width, still doesn't look correct on small screens but might be better when we get mobile-specific font sizes?

Removed min-width, still doesn't look correct on small screens but might be better when we get mobile-specific font sizes?
<div key={`${datum.name}-${index}`} className={styles.navItem}>
<a href={datum.url}>
<h4>{String(index).padStart(2, "0")} </h4>
<p>{datum.name}</p>
Owner

Just looking at this markup more closely, some thoughts:

  • I don't think we usually put tags like h4 and p inside a? Maybe we can get rid of the h4 and p, then use spans to style specific text.
  • Let's wrap the as in lis instead of divs, and throw them in an ol or ul inside of the nav. So the nesting would be like
<nav>
  <ol>
    <li>
      <a>...</a>
    </li>
  </ol>
</nav>
Just looking at this markup more closely, some thoughts: - I don't think we usually put tags like `h4` and `p` inside `a`? Maybe we can get rid of the `h4` and `p`, then use spans to style specific text. - Let's wrap the `a`s in `li`s instead of `div`s, and throw them in an `ol` or `ul` inside of the nav. So the nesting would be like ```html <nav> <ol> <li> <a>...</a> </li> </ol> </nav> ```
j285he marked this conversation as resolved
j285he added 2 commits 3 months ago
j285he added 2 commits 3 months ago
cbc7be0661 Change about default export
j285he added 1 commit 3 months ago
3da9cf1dc6 Change heights
Poster
Collaborator

I changed the heights, do I need a re-approval @a258wang ?

I changed the heights, do I need a re-approval @a258wang ?
a258wang approved these changes 3 months ago
j285he added 1 commit 3 months ago
34e05db3fc Merge main
j285he merged commit 8253f6cbab into main 3 months ago
j285he deleted branch j285he-sections 3 months ago
j285he referenced this issue from a commit 3 months ago

Reviewers

snedadah was requested for review 3 months ago
e26chiu was requested for review 3 months ago
b72zhou was requested for review 3 months ago
a258wang approved these changes 3 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 8253f6cbab.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#49
Loading…
There is no content yet.