Add Sections Component #49

j285he · 2022-09-01T02:10:32-04:00

j285he commented

2022-09-01 02:10:32 -04:00

Closes #43.

Will look better when !46 is merged in.

https://j285he-sections-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/

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 2022-09-01 02:10:33 -04:00

continuous-integration/drone/push Build is failing Details

d0dc94441d Initial sections commit

continuous-integration/drone/push Build is passing Details

ff2ec0ce3b Add Sections

j285he requested review from a258wang 2022-09-01 02:10:42 -04:00

j285he requested review from snedadah 2022-09-01 02:10:47 -04:00

j285he requested review from e26chiu 2022-09-01 02:10:50 -04:00

j285he requested review from b72zhou 2022-09-01 02:10:53 -04:00

snedadah reviewed 2022-09-02 18:12:59 -04:00

components/Sections.tsx

													
				@ -0,0 +14,4 @@

				export default function Sections({ data }: SectionsProps) {

				  return (

				    <section className={styles.sections}>

				      <h1>Sections</h1>

snedadah commented

2022-09-02 18:12:28 -04:00

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

components/Sections.tsx Outdated

													
				@ -0,0 +20,4 @@

				        {data.map((datum, index) => {

				          return (

				            <div key={`${datum.name}-${index}`} className={styles.navItem}>

				              <h4>{String(index).padStart(2, "0")}</h4>

snedadah commented

2022-09-02 18:10:20 -04:00

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

a258wang commented

2022-09-07 20:39:44 -04:00

I think having the numbers be clickable might be nice!

j285he marked this conversation as resolved

snedadah reviewed 2022-09-04 01:40:51 -04:00

components/Sections.tsx

													
				@ -0,0 +11,4 @@

				  data: SectionsData[];

				}

				export default function Sections({ data }: SectionsProps) {

snedadah commented

2022-09-04 01:40:51 -04:00

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 2022-09-07 20:41:02 -04:00

components/Sections.module.css Outdated

													
				@ -0,0 +1,52 @@

				.sections {

				  display: flex;

				  flex-direction: row;

				  width: 50%;

a258wang commented

2022-09-07 20:38:00 -04:00

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`?

j285he commented

2022-09-07 22:59:32 -04:00

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.

a258wang commented

2022-09-09 12:51:51 -04:00

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.

components/Sections.module.css Outdated

													
				@ -0,0 +2,4 @@

				  display: flex;

				  flex-direction: row;

				  width: 50%;

				  gap: 15px;

a258wang commented

2022-09-07 20:37:18 -04:00

NIT: calc(15rem / 16)

NIT: `calc(15rem / 16)`

j285he marked this conversation as resolved

components/Sections.module.css Outdated

													
				@ -0,0 +30,4 @@

				  display: flex;

				  flex-direction: row;

				  align-items: flex-start;

				  margin: calc(20rem / 16);

a258wang commented

2022-09-07 20:39:13 -04:00

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

components/Sections.module.css Outdated

													
				@ -0,0 +47,4 @@

				}

				.nav a:hover {

				  color: var(--primary-accent-light);

a258wang commented

2022-09-07 20:36:45 -04:00

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

data/routes.ts

													
				@ -0,0 +1,46 @@

				export const sectionsData = [

a258wang commented

2022-09-07 20:40:48 -04:00

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.

j285he commented

2022-09-07 22:38:52 -04:00

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

👍 1

a258wang marked this conversation as resolved

j285he added 1 commit 2022-09-07 22:59:49 -04:00

continuous-integration/drone/push Build is passing Details

5c87e7636e Code review fixes

j285he added 2 commits 2022-09-07 23:01:54 -04:00

e04ccbefc2 Merge main

continuous-integration/drone/push Build is passing Details

7f64de74b7 Merge main

j285he commented

2022-09-07 23:03:45 -04:00

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 2022-09-07 23:08:38 -04:00

continuous-integration/drone/push Build is passing Details

c9522ece4b Remove margin: 0 auto

a258wang approved these changes 2022-09-09 13:05:12 -04:00

a258wang left a comment

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.

components/Sections.module.css

													
				@ -0,0 +1,54 @@

				.sections {

				  display: flex;

				  flex-direction: row;

				  min-width: calc(1000rem / 16);

a258wang commented

2022-09-09 13:02:59 -04:00

Continuing from thoughts here: #49 (comment)

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.

j285he commented

2022-09-09 17:23:55 -04:00

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

👍 1

components/Sections.tsx Outdated

													
				@ -0,0 +30,4 @@

				            <div key={`${datum.name}-${index}`} className={styles.navItem}>

				              <a href={datum.url}>

				                <h4>{String(index).padStart(2, "0")} </h4>

				                <p>{datum.name}</p>

a258wang commented

2022-09-09 13:03:12 -04:00

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 2022-09-09 17:29:31 -04:00

7c9cb7b8c8 Merge branch 'main' into j285he-sections

continuous-integration/drone/push Build is passing Details

376c540faa Code review fixes

j285he added 2 commits 2022-09-12 10:24:36 -04:00

8465b5be44 Merge main

continuous-integration/drone/push Build is passing Details

cbc7be0661 Change about default export

j285he added 1 commit 2022-09-12 10:57:54 -04:00

continuous-integration/drone/push Build is passing Details

3da9cf1dc6 Change heights