Add Sections Component #49

Merged
j285he merged 12 commits from j285he-sections into main 2022-09-12 20:07:05 -04:00
5 changed files with 170 additions and 11 deletions

View File

@ -2,7 +2,7 @@ import React from "react";
import styles from "./About.module.css";
export default function About() {
export function About() {
return (
<div className={styles.aboutWrapper}>
<AngleDecoration isBottom={false} />

View File

@ -0,0 +1,60 @@
.sections {
display: flex;
flex-direction: row;
gap: calc(15rem / 16);

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

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.

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.
Review

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.
Review

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?
}
j285he marked this conversation as resolved Outdated

NIT: calc(15rem / 16)

NIT: `calc(15rem / 16)`
.sections h1 {
flex: 3;
text-align: right;
margin: 0;
}
.separator {
flex: 1;
background-color: var(--label);
height: calc(1rem / 16);
width: 100%;
margin-top: calc(30rem / 16);
}
.nav {
flex: 3;
display: flex;
flex-direction: column;
}
.nav ul {
list-style-type: none;
margin: 0;
padding: 0;
}
.nav li {
j285he marked this conversation as resolved Outdated

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.
margin: calc(20rem / 16);
margin-left: 0;
}
.nav li:first-child {
margin-top: calc(18rem / 16);
}
.nav li .linkNumber {
color: var(--secondary-accent);
margin: 0;
display: inline;
}
.nav li a {
font-size: calc(24rem / 16);
color: var(--primary-text);
j285he marked this conversation as resolved Outdated

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.)
}
.nav li a:hover .linkName {
text-decoration: underline;
}
.nav li .linkName {
margin: 0;
display: inline;
}

45
components/Sections.tsx Normal file
View File

@ -0,0 +1,45 @@
import React from "react";
import styles from "./Sections.module.css";
interface SectionsData {
name: string;
url: string;
}
interface SectionsProps {
/* Whether to display the "Sections" title and separator that appears on the left. */
showHeader?: boolean;
data: SectionsData[];
}
j285he marked this conversation as resolved
Review

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.
export function Sections({ data, showHeader = true }: SectionsProps) {
return (
j285he marked this conversation as resolved
Review

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.
<section className={styles.sections}>
{showHeader ? (
<>
<h1>Sections</h1>
<div className={styles.separator} />
</>
j285he marked this conversation as resolved Outdated

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?

I think having the numbers be clickable might be nice!

I think having the numbers be clickable might be nice!
) : (
""
)}
<nav className={styles.nav}>
<ul>
{data.map((datum, index) => {
return (
<li key={`${datum.name}-${index}`}>
<a href={datum.url}>
<span className={styles.linkNumber}>
j285he marked this conversation as resolved Outdated

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> ```
{String(index).padStart(2, "0")}{" "}
</span>
<span className={styles.linkName}>{datum.name}</span>
</a>
</li>
);
})}
</ul>
</nav>
</section>
);
}

46
data/routes.ts Normal file
View File

@ -0,0 +1,46 @@
export const sectionsData = [
a258wang marked this conversation as resolved
Review

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.
Review

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
{
name: "Demographics",
url: "/",
},
{
name: "Academics",
url: "/",
},
{
name: "Co-op",
url: "/",
},
{
name: "Lifestyle and Interests",
url: "/",
},
{
name: "Intimacy and Drugs",
url: "/",
},
{
name: "Post-grad",
url: "/",
},
{
name: "Friends",
url: "/",
},
{
name: "Miscellaneous",
url: "/",
},
{
name: "Mental Health",
url: "/",
},
{
name: "Personal",
url: "/",
},
{
name: "Contributors",
url: "/",
},
];

View File

@ -9,11 +9,13 @@ import {
mockPieData,
mockTimelineData,
} from "data/mocks";
import { sectionsData } from "data/routes";
import React from "react";
import About from "@/components/About";
import { About } from "@/components/About";
import { PieChart } from "@/components/PieChart";
import { QuotationCarousel } from "@/components/QuotationCarousel";
import { Sections } from "@/components/Sections";
import { Timeline } from "@/components/Timeline";
import { CenterWrapper } from "../components/CenterWrapper";
@ -27,14 +29,8 @@ export default function Home() {
<div className={styles.page}>
<h1>Playground</h1>
<p>Show off your components here!</p>
<ColorPalette />
<h2>
<code>{"<PieChart />"}</code>
</h2>
<div style={{ padding: "30px" }}>
<PieChart data={mockPieData} width={800} labelWidth={215} />
</div>
<ColorPalette />
<h2>
<code>Text Styles</code>
@ -46,6 +42,13 @@ export default function Home() {
<p>p p p p p p p p p p p p p p p p p p p p p p p p p p p p</p>
<a href="#">a a a a a a a a a a a a a a a a a a a a a a a a a a a a</a>
<h2>
<code>{"<PieChart />"}</code>
</h2>
<div style={{ padding: "30px" }}>
<PieChart data={mockPieData} width={800} labelWidth={215} />
</div>
<h2>
<code>{"<BarGraphHorizontal />"}</code>
</h2>
@ -130,9 +133,14 @@ export default function Home() {
</CenterWrapper>
<h2>
<code>{"<About />"}</code>
<About />
<code>{"<Sections />"}</code>
</h2>
<Sections data={sectionsData} />
<h2>
<code>{"<About />"}</code>
</h2>
<About />
<h2>
<code>{"<BoxPlot />"}</code>