www
/
cs-2022-class-profile
20
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

Sample page and graph wrappers #32

Merged
snedadah merged 18 commits from shahanneda/wrapper into main 2022-09-02 17:39:47 -04:00
Conversation 0 Commits 18 Files Changed 12 +134 -37
9 changed files with 134 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 8a85e3c5d9 - Show all commits

21
components/FullComponentWrapper.module.css Normal file
View File

@ -0,0 +1,21 @@
.wrapper {
align-items: center;
justify-content: center;
display: flex;
flex-direction: column;
text-align: center;
width: 80%;
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:20:18 -04:00
Outdated
Review
Copy Link

Instead of specifying the width as a percentage, what if we tried

max-width: ...
margin: auto;
padding: /* however much spacing you want */

This will center the box horizontally (so no need for the outer container to be a flexbox or have align-self: center) and also respond reasonably well as the screen size changes.

Instead of specifying the width as a percentage, what if we tried ```css max-width: ... margin: auto; padding: /* however much spacing you want */ ``` This will center the box horizontally (so no need for the outer container to be a flexbox or have `align-self: center`) and also respond reasonably well as the screen size changes.
align-self: center;
margin: calc(40rem / 16) 0;
}
.graphWrapper {
margin-top: calc(80rem / 16);
a258wang marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:26:21 -04:00
Outdated
Review
Copy Link

NIT: could use gap (and maybe space-between?) on the flexbox instead of margin here

NIT: could use `gap` (and maybe `space-between`?) on the flexbox instead of margin here
snedadah commented 2022-08-11 23:08:53 -04:00
Outdated
Review
Copy Link

I changed this to padding, which is better for what I was trying to accomplish here. I also tried out gap/space-between both those didn't seem to help in this case.

I changed this to padding, which is better for what I was trying to accomplish here. I also tried out gap/space-between both those didn't seem to help in this case.
👍 1
}
/* .textWrapper {
margin: 0 calc(100rem / 16);
} */

25
components/FullComponentWrapper.tsx Normal file
View File

@ -0,0 +1,25 @@
import React from "react";
import styles from "./FullComponentWrapper.module.css";
type FullComponentWrapperProps = {
children: React.ReactNode;
heading: string;
body: string;
};
export default function FullComponentWrapper({
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:28:02 -04:00
Outdated
Review
Copy Link

Our convention is usually to make component exports non-default.

Also, NIT: would it make more sense to call this CenterComponentWrapper or something involving the word "center"?

Our convention is usually to make component exports non-default. Also, NIT: would it make more sense to call this `CenterComponentWrapper` or something involving the word "center"?
heading,
body,
children,
}: FullComponentWrapperProps) {
return (
<div className={styles.wrapper}>
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:21:02 -04:00
Outdated
Review
Copy Link

Would it make sense to use a section instead of a div? (If so, it'd also probably make sense to just use an h1 inside the section.)

Would it make sense to use a `section` instead of a `div`? (If so, it'd also probably make sense to just use an `h1` inside the `section`.)
snedadah commented 2022-08-11 23:10:15 -04:00
Outdated
Review
Copy Link

Our h1 should be for the page header, (I think thats better semantic html) and also we already have setup the corresponding colors for it. H1/H2 has the header/sub-header color, while the h3 has the color we want here. Is there a reason to prefer h1's inside a section?

Our h1 should be for the page header, (I think thats better semantic html) and also we already have setup the corresponding colors for it. H1/H2 has the header/sub-header color, while the h3 has the color we want here. Is there a reason to prefer h1's inside a section?
a258wang commented 2022-08-24 19:53:26 -04:00
Outdated
Review
Copy Link

Oops, I made a mistake, turns out it's best practice to have a header inside a section, but it doesn't have to be an h1.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

On second thought, I'm not 100% sure we need each wrapper to be its own section, so this is probably fine. 🙂

Oops, I made a mistake, turns out it's best practice to have a header inside a section, but it doesn't have to be an h1. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section On second thought, I'm not 100% sure we need each wrapper to be its own section, so this is probably fine. 🙂
<div className={styles.textWrapper}>
<h3 className={styles.heading}>{heading}</h3>
<p className={styles.body}>{body}</p>
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:23:23 -04:00
Outdated
Review
Copy Link

These classes don't appear to be declared in the CSS module, please remove them if they're not needed

These classes don't appear to be declared in the CSS module, please remove them if they're not needed
</div>
<div className={styles.graphWrapper}>{children}</div>
</div>
);
}

10
components/SideComponentWrapper.module.css
View File

@ -1,9 +1,9 @@
.wrapperCommon { .wrapperCommon {
background-color: var(--secondary-background); background-color: var(--secondary-background);
display: flex; display: flex;
width: 80%; width: 95%;
a258wang commented 2022-08-11 20:39:02 -04:00
Outdated
Review
Copy Link

I would suggest taking a look at the Bubble component from the website for inspiration (TSX, CSS).

In particular, we should probably turn the "bubble" into just a coloured bar on smaller screens, similar to what we have on the website's About Us page.

I would suggest taking a look at the `Bubble` component from the website for inspiration ([TSX](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/components/Bubble.tsx), [CSS](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/components/Bubble.module.css)). In particular, we should probably turn the "bubble" into just a coloured bar on smaller screens, similar to what we have on the website's About Us page.
snedadah commented 2022-08-11 23:25:59 -04:00
Outdated
Review
Copy Link

Took a look at the Bubble, I matched the solid bar on mobile similar to that. I don't think we need the alternating automatic left/right align for our usecase, but let me know if you think it would be useful. I don't think it would work that well especially since I now added a side component with no background which we would not want to change the left/right pattern.

In terms of how the bubble is implmented there, it is using a compltely different alternative technique to make the bubbles (background and are content different elements, background is transformed) compared that what I did here, so I can't really apply those ideas here without rewriting it from scratch.

Took a look at the Bubble, I matched the solid bar on mobile similar to that. I don't think we need the alternating automatic left/right align for our usecase, but let me know if you think it would be useful. I don't think it would work that well especially since I now added a side component with no background which we would not want to change the left/right pattern. In terms of how the bubble is implmented there, it is using a compltely different alternative technique to make the bubbles (background and are content different elements, background is transformed) compared that what I did here, so I can't really apply those ideas here without rewriting it from scratch.
padding: calc(40rem / 16) calc(50rem / 16); padding: calc(40rem / 16) calc(50rem / 16);
margin: calc(80rem / 16) 0; margin: calc(40rem / 16) 0;
} }
.wrapperRight { .wrapperRight {
@ -22,6 +22,12 @@
border-radius: 0 12.5rem 12.5rem 0; border-radius: 0 12.5rem 12.5rem 0;
} }
@media screen and (max-width: 768px) {
.wrapperCommon {
flex-direction: column;
}
}
.graphWrapper { .graphWrapper {
margin: calc(20rem / 16); margin: calc(20rem / 16);
} }

2
components/SideComponentWrapper.tsx
View File

@ -17,11 +17,11 @@ export default function SideComponentWrapper({
}: ComponentWrapperProps) { }: ComponentWrapperProps) {
return ( return (
<div className={rightAligned ? styles.wrapperRight : styles.wrapperLeft}> <div className={rightAligned ? styles.wrapperRight : styles.wrapperLeft}>
<div className={styles.graphWrapper}>{children}</div>
<div className={styles.textWrapper}> <div className={styles.textWrapper}>
<h3 className={styles.heading}>{heading}</h3> <h3 className={styles.heading}>{heading}</h3>
<p className={styles.body}>{body}</p> <p className={styles.body}>{body}</p>
</div> </div>
a258wang marked this conversation as resolved Outdated
a258wang commented 2022-08-11 20:44:34 -04:00
Outdated
Review
Copy Link

Would it work if we set flex-direction: row vs flex-direction: row-reverse for the left/right wrapper classes, instead of having this awkward conditional thing here?

Would it work if we set `flex-direction: row` vs `flex-direction: row-reverse` for the left/right wrapper classes, instead of having this awkward conditional thing here?
snedadah commented 2022-08-11 23:26:55 -04:00
Outdated
Review
Copy Link

Great point! totally forgot those were a thing

Great point! totally forgot those were a thing
<div className={styles.graphWrapper}>{children}</div>
</div> </div>
); );
} }

13
pages/_app.tsx
View File

@ -1,9 +1,20 @@
import type { AppProps } from "next/app"; import type { AppProps } from "next/app";
import Head from "next/head";
import React from "react"; import React from "react";
import "./_app.css"; import "./_app.css";
import "./font.css"; import "./font.css";
export default function App({ Component, pageProps }: AppProps): JSX.Element { export default function App({ Component, pageProps }: AppProps): JSX.Element {
return <Component {...pageProps} />; return (
<>
<Head>
snedadah commented 2022-08-09 01:03:03 -04:00
Review
Copy Link

For making all pages responsive

For making all pages responsive
a258wang commented 2022-08-11 21:04:53 -04:00
Review
Copy Link

Is this required? (I'm guessing it's needed for react-responsive?)

Is this required? (I'm guessing it's needed for react-responsive?)
snedadah commented 2022-08-11 23:30:50 -04:00
Review
Copy Link

This is needed for making any html page responsive! (has nothing to do with react-responsive)
https://www.w3schools.com/html/html_responsive.asp

This is needed for making any html page responsive! (has nothing to do with react-responsive) https://www.w3schools.com/html/html_responsive.asp
a258wang commented 2022-08-24 19:21:51 -04:00
Review
Copy Link

Today I learned, LOL. Though it doesn't seem like we have this setup for the website and the responsive shenanigans work fine?

Today I learned, LOL. Though it doesn't seem like we have this setup for the website and the responsive shenanigans work fine?
snedadah commented 2022-08-31 07:14:54 -04:00
Review
Copy Link

I'm not sure how its setup in the site (I also couldn't find the code manually being set), however, it definitely is set:

if you view source of the site: view-source:https://csclub.uwaterloo.ca/
it's one of the first things there.
<!DOCTYPE html><html lang="en"><head><meta name="viewport" content="width=device-width"/><meta charSet="utf-8"/><title>CSC - University of Wate ...

I wonder if it is a Next option or something (however I coulnd't find it)

I'm not sure how its setup in the site (I also couldn't find the code manually being set), however, it definitely is set: if you view source of the site: view-source:https://csclub.uwaterloo.ca/ it's one of the first things there. ``` <!DOCTYPE html><html lang="en"><head><meta name="viewport" content="width=device-width"/><meta charSet="utf-8"/><title>CSC - University of Wate ...``` I wonder if it is a Next option or something (however I coulnd't find it)
a258wang commented 2022-09-02 01:09:17 -04:00
Review
Copy Link

Ahh yeah sorry I should've clarified, I meant that we didn't set up anything manually for the website, but the viewport meta tag somehow got added anyways.

I'm guessing you already tried responsive shenanigans in the class profile without manually adding in the stuff in the head here? If it works without this here, then we can get rid of it, but if it doesn't work, then we can keep it.

Ahh yeah sorry I should've clarified, I meant that we didn't set up anything manually for the website, but the viewport meta tag somehow got added anyways. I'm guessing you already tried responsive shenanigans in the class profile without manually adding in the stuff in the head here? If it works without this here, then we can get rid of it, but if it doesn't work, then we can keep it.
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, maximum-scale=1.0,user-scalable=0"
/>
</Head>
<Component {...pageProps} />;
</>
);
} }

2
pages/index.tsx
View File

@ -5,6 +5,8 @@ export default function Home() {
return ( return (
<p> <p>
Click <Link href="/playground">here</Link> to visit the playground Click <Link href="/playground">here</Link> to visit the playground
<br></br>
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 21:05:12 -04:00
Outdated
Review
Copy Link

NIT: <br /> 😆

NIT: `<br />` 😆
Click <Link href="/samplePage">here</Link> to visit a sample page
</p> </p>
); );
} }

29
pages/playground.tsx
View File

@ -62,35 +62,6 @@ export default function Home() {
}))} }))}
/> />
<SideComponentWrapper
heading="What program are you in?"
body="There are a total of 106 respondents of the CS Class Profile. Interestingly, there are a huge number of students that are just in CS, partially due to the overwhelming number of people in CS as seen in the total demographics."
>
<BarGraphVertical
data={mockCategoricalData}
width={800}
height={500}
margin={{
top: 20,
bottom: 80,
left: 60,
right: 20,
}}
/>
</SideComponentWrapper>
<SideComponentWrapper
heading="What program are you in?"
body="There are a total of 106 respondents of the CS Class Profile. Interestingly, there are a huge number of students that are just in CS, partially due to the overwhelming number of people in CS as seen in the total demographics."
rightAligned
>
<WordCloud
data={moreMockCategoricalData.map((word) => ({
text: word.key,
value: word.value,
}))}
/>
</SideComponentWrapper>
</div> </div>
); );
} }

32
pages/samplePage.tsx
View File

@ -1,7 +1,9 @@
import { BarGraphVertical } from "components/BarGraph"; import { BarGraphHorizontal, BarGraphVertical } from "components/BarGraph";
import { mockCategoricalData, moreMockCategoricalData } from "data/mocks"; import { mockCategoricalData, moreMockCategoricalData } from "data/mocks";
import React from "react"; import React from "react";
import useWindowDimensions from "utils/getWindowDimensions";
import FullComponentWrapper from "@/components/FullComponentWrapper";
import SideComponentWrapper from "@/components/SideComponentWrapper"; import SideComponentWrapper from "@/components/SideComponentWrapper";
import { WordCloud } from "../components/WordCloud"; import { WordCloud } from "../components/WordCloud";
@ -9,6 +11,8 @@ import { WordCloud } from "../components/WordCloud";
import styles from "./samplePage.module.css"; import styles from "./samplePage.module.css";
export default function SamplePage() { export default function SamplePage() {
const { height, width } = useWindowDimensions();
return ( return (
<div className={styles.page}> <div className={styles.page}>
<SideComponentWrapper <SideComponentWrapper
@ -17,7 +21,7 @@ export default function SamplePage() {
> >
<BarGraphVertical <BarGraphVertical
data={mockCategoricalData} data={mockCategoricalData}
width={800} width={700}
height={500} height={500}
margin={{ margin={{
top: 20, top: 20,
@ -28,16 +32,36 @@ export default function SamplePage() {
/> />
</SideComponentWrapper> </SideComponentWrapper>
<SideComponentWrapper <FullComponentWrapper
heading="What program are you in?" heading="What program are you in?"
body="There are a total of 106 respondents of the CS Class Profile. Interestingly, there are a huge number of students that are just in CS, partially due to the overwhelming number of people in CS as seen in the total demographics." body="There are a total of 106 respondents of the CS Class Profile. Interestingly, there are a huge number of students that are just in CS, partially due to the overwhelming number of people in CS as seen in the total demographics."
rightAligned
> >
<WordCloud <WordCloud
data={moreMockCategoricalData.map((word) => ({ data={moreMockCategoricalData.map((word) => ({
text: word.key, text: word.key,
value: word.value, value: word.value,
}))} }))}
width={1000}
height={500}
/>
</FullComponentWrapper>
<SideComponentWrapper
heading="What program are you in?"
body="There are a total of 106 respondents of the CS Class Profile. Interestingly, there are a huge number of students that are just in CS, partially due to the overwhelming number of people in CS as seen in the total demographics."
rightAligned
>
<BarGraphHorizontal
className={styles.barGraphDemo}
data={mockCategoricalData}
width={width / 2}
height={500}
margin={{
top: 20,
bottom: 40,
left: 150,
right: 20,
}}
/> />
</SideComponentWrapper> </SideComponentWrapper>
</div> </div>

37
utils/getWindowDimensions.ts Normal file
View File

@ -0,0 +1,37 @@
// https://stackoverflow.com/questions/36862334/get-viewport-window-height-in-reactjs
snedadah marked this conversation as resolved
a258wang commented 2022-08-11 21:14:13 -04:00
Review
Copy Link

I'm pretty sure we referenced the same thing when working on the website, but we definitely didn't cite our sources like this 😂

I'm pretty sure we referenced the same thing when working on the website, but we definitely didn't cite our sources like this 😂
import { useState, useEffect } from "react";
type WindowDimensions = {
width: number;
height: number;
};
const getWindowDimensions = (): WindowDimensions => {
const { innerWidth, innerHeight } = window;
return {
width: innerWidth,
height: innerHeight,
};
};
const useWindowDimensions = (): WindowDimensions => {
const [windowDimensions, setWindowDimensions] = useState<WindowDimensions>({
width: 0,
height: 0,
});
const handleResize = () => {
setWindowDimensions(getWindowDimensions());
};
useEffect(() => {
handleResize();
window.addEventListener("resize", handleResize);
return () => window.removeEventListener("resize", handleResize);
}, []);
return windowDimensions;
};
export default useWindowDimensions;
snedadah marked this conversation as resolved Outdated
a258wang commented 2022-08-11 21:12:01 -04:00
Outdated
Review
Copy Link

NIT: make this non-default

NIT: make this non-default