Sample page and graph wrappers #32
|
@ -2,12 +2,13 @@
|
|||
"typescript.tsdk": "node_modules/typescript/lib",
|
||||
"eslint.format.enable": true,
|
||||
"eslint.codeActionsOnSave.mode": "all",
|
||||
"css.lint.validProperties": ["composes"],
|
||||
a258wang marked this conversation as resolved
|
||||
"css.format.spaceAroundSelectorSeparator": true,
|
||||
"[css]": {
|
||||
"editor.suggest.insertMode": "replace",
|
||||
"gitlens.codeLens.scopes": ["document"],
|
||||
"editor.formatOnSave": true,
|
||||
"editor.defaultFormatter": "vscode.css-language-features"
|
||||
"editor.defaultFormatter": "vscode.css-language-features",
|
||||
},
|
||||
"[javascript]": {
|
||||
"editor.formatOnSave": false,
|
||||
|
|
|
@ -0,0 +1,59 @@
|
|||
.sideWrapperCommon {
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
NIT: maybe rename this to something like NIT: maybe rename this to something like `sideWrapperCommon` or just `sideWrapper`, since it's only common to the left and right wrappers?
|
||||
background-color: var(--secondary-background);
|
||||
display: flex;
|
||||
padding: calc(40rem / 16) calc(50rem / 16);
|
||||
margin: calc(65rem / 16) 0;
|
||||
width: 90%;
|
||||
}
|
||||
|
||||
.wrapperRight {
|
||||
composes: sideWrapperCommon;
|
||||
align-self: end;
|
||||
margin-right: 0;
|
||||
padding-right: 0;
|
||||
border-radius: calc(200rem / 16) 0 0 calc(200rem / 16);
|
||||
flex-direction: row-reverse;
|
||||
padding-right: calc(50rem / 16);
|
||||
}
|
||||
|
||||
.wrapperLeft {
|
||||
composes: sideWrapperCommon;
|
||||
align-self: start;
|
||||
margin-left: 0;
|
||||
padding-left: 0;
|
||||
border-radius: 0 calc(200rem / 16) calc(200rem / 16) 0;
|
||||
flex-direction: row;
|
||||
padding-left: calc(50rem / 16);
|
||||
}
|
||||
|
||||
.noBackground {
|
||||
background: none;
|
||||
align-self: center;
|
||||
}
|
||||
|
||||
.wrapperCenter {
|
||||
flex-direction: column;
|
||||
text-align: center;
|
||||
gap: calc(25rem / 16);
|
||||
/* to match the 65px margin with the left/right variant:
|
||||
add 45px bottom margin, since internal wrapper contributes 20px for the center component
|
||||
0px top margin, since h3 contributes 45px and internal wrapper contributes 20px for the center component
|
||||
*/
|
||||
margin: 0 0 calc(45rem / 16) 0;
|
||||
padding: 0 15%;
|
||||
}
|
||||
|
||||
@media screen and (max-width: 768px) {
|
||||
.sideWrapperCommon {
|
||||
margin: auto;
|
||||
flex-direction: column;
|
||||
text-align: center;
|
||||
padding: 0;
|
||||
border-radius: 0;
|
||||
width: 100%;
|
||||
}
|
||||
}
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
NIT: unnecessary empty line :> NIT: unnecessary empty line :>
|
||||
|
||||
.internalWrapper {
|
||||
padding: calc(20rem / 16);
|
||||
}
|
|
@ -0,0 +1,42 @@
|
|||
import React from "react";
|
||||
|
||||
import styles from "./ComponentWrapper.module.css";
|
||||
|
||||
type AlignOption = "left" | "center" | "right";
|
||||
|
||||
type ComponentWrapperProps = {
|
||||
children: React.ReactNode;
|
||||
heading: string;
|
||||
bodyText: string;
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
NIT: maybe rename to Another option to consider could be having the graph be a prop (ie. Actually now I'm wondering if it'd make sense to call this component NIT: maybe rename to `bodyText` or just `text`? Just to differentiate between what should be the "children" vs the "body".
Another option to consider could be having the graph be a prop (ie. `graph: React.ReactNode`) instead of just children. The advantage is that naming the prop like so makes it more clear how this component is intended to be used from just the interface alone, but the disadvantage is that it could make using the ComponentWrapper component a little bit more ugly.
Actually now I'm wondering if it'd make sense to call this component `GraphWrapper` instead of just `ComponentWrapper` 🤔
snedadah
commented
I mean the wrapper could also wrap things other than graphs too (just whatever is inside the children) so I don't think it should be called a GraphWrapper. I mean the wrapper could also wrap things other than graphs too (just whatever is inside the children) so I don't think it should be called a GraphWrapper.
|
||||
align?: AlignOption;
|
||||
noBackground?: boolean;
|
||||
};
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
unused prop unused prop
|
||||
|
||||
export function ComponentWrapper({
|
||||
heading,
|
||||
bodyText,
|
||||
children,
|
||||
align = "left",
|
||||
noBackground = false,
|
||||
}: ComponentWrapperProps) {
|
||||
const alignClasses: { [key in AlignOption]: string } = {
|
||||
left: styles.wrapperLeft,
|
||||
center: styles.wrapperCenter,
|
||||
right: styles.wrapperRight,
|
||||
};
|
||||
|
||||
return (
|
||||
<div
|
||||
className={`
|
||||
${alignClasses[align]}
|
||||
${noBackground ? styles.noBackground : ""}
|
||||
`}
|
||||
>
|
||||
<div className={styles.internalWrapper}>
|
||||
<h3>{heading}</h3>
|
||||
<p>{bodyText}</p>
|
||||
</div>
|
||||
<div className={styles.internalWrapper}>{children}</div>
|
||||
</div>
|
||||
);
|
||||
}
|
|
@ -197,10 +197,12 @@ const shouldNotRerender = (
|
|||
nextProps: WordCloudWordsProps
|
||||
) => {
|
||||
if (
|
||||
prevProps.tooltipLeft !== nextProps.tooltipLeft ||
|
||||
prevProps.tooltipTop !== nextProps.tooltipTop ||
|
||||
nextProps.tooltipLeft === undefined ||
|
||||
nextProps.tooltipTop === undefined
|
||||
// if width changes, rerender, else don't rerender for a tooltip change
|
||||
prevProps.width === nextProps.width &&
|
||||
(prevProps.tooltipLeft !== nextProps.tooltipLeft ||
|
||||
prevProps.tooltipTop !== nextProps.tooltipTop ||
|
||||
nextProps.tooltipLeft === undefined ||
|
||||
nextProps.tooltipTop === undefined)
|
||||
) {
|
||||
return true; // do not re-render
|
||||
}
|
||||
|
|
|
@ -21,9 +21,9 @@
|
|||
"@visx/group": "^2.10.0",
|
||||
"@visx/scale": "^2.2.2",
|
||||
"@visx/shape": "^2.10.0",
|
||||
"@visx/text": "^2.10.0",
|
||||
"@visx/tooltip": "^2.10.0",
|
||||
"@visx/wordcloud": "^2.10.0",
|
||||
"@visx/text": "^2.10.0",
|
||||
"next": "12.1.6",
|
||||
"react": "18.1.0",
|
||||
"react-dom": "18.1.0"
|
||||
|
|
|
@ -73,7 +73,11 @@ h2 {
|
|||
color: var(--primary-heading);
|
||||
}
|
||||
|
||||
h3,
|
||||
h3 {
|
||||
color: var(--secondary-heading);
|
||||
font-size: calc(45rem / 16);
|
||||
}
|
||||
|
||||
h4,
|
||||
h5,
|
||||
h6 {
|
||||
|
@ -89,6 +93,10 @@ a:hover {
|
|||
color: var(--link-hover);
|
||||
}
|
||||
|
||||
p {
|
||||
font-size: calc(28rem / 16);
|
||||
a258wang marked this conversation as resolved
a258wang
commented
This font is so big 😂 Let's create an issue to standardize the font sizes to be reasonable across the site Edit: #33 This font is so big 😂 Let's create an issue to standardize the font sizes to be reasonable across the site
Edit: #33
snedadah
commented
Leaving it as it is for now, we can handle it later in that issue. Leaving it as it is for now, we can handle it later in that issue.
|
||||
}
|
||||
|
||||
@media only screen and (prefers-color-scheme: dark) {
|
||||
body {
|
||||
--primary-background: var(--dark--primary-background);
|
||||
|
|
|
@ -1,9 +1,20 @@
|
|||
import type { AppProps } from "next/app";
|
||||
import Head from "next/head";
|
||||
import React from "react";
|
||||
|
||||
import "./_app.css";
|
||||
import "./font.css";
|
||||
|
||||
export default function App({ Component, pageProps }: AppProps): JSX.Element {
|
||||
return <Component {...pageProps} />;
|
||||
return (
|
||||
<>
|
||||
<Head>
|
||||
snedadah
commented
For making all pages responsive For making all pages responsive
a258wang
commented
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
This is needed for making any html page responsive! (has nothing to do with react-responsive) 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
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
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/ 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
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} />
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
|
|
@ -5,6 +5,8 @@ export default function Home() {
|
|||
return (
|
||||
<p>
|
||||
Click <Link href="/playground">here</Link> to visit the playground
|
||||
<br />
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
NIT: NIT: `<br />` 😆
|
||||
Click <Link href="/samplePage">here</Link> to visit a sample page
|
||||
</p>
|
||||
);
|
||||
}
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
.page {
|
||||
display: flex;
|
||||
a258wang
commented
Would be nice to not have to have the page be a flexbox, but I think I'm just being picky at this point LOL (But if we're really sticking to a flex-direction column flexbox, maybe also consider setting Would be nice to not have to have the page be a flexbox, but I think I'm just being picky at this point LOL
(But if we're really sticking to a flex-direction column flexbox, maybe also consider setting `justify-content: center`)
snedadah
commented
Is there a reason for not using flexbox? The alternative is to use "float:right", on the bubbles, instead of "align-self: end", so I just stuck with making everything flex. Is there a reason for not using flexbox? The alternative is to use "float:right", on the bubbles, instead of "align-self: end", so I just stuck with making everything flex.
a258wang
commented
My rationale for trying to avoid flexbox here is that this imposes an additional requirement on the outer container, which is separate from the Wrapper component... so inevitably someone someday is going to try to use the Wrapper component and be like "wHy DoEsN't ThIs WoRk?!!!" because they forgot to set The alternative to both flexbox and float is what's used on the website: the Bubble component consists of a relatively positioned "container" with an absolutely positioned "bar" (what we see as the "bubble") that's translated sideways and cut off at the edge of the screen. This way, devs don't need to remember to do anything extra when using the component, because everything that's required for the component to work properly is an internal detail. My rationale for trying to avoid flexbox here is that this imposes an additional requirement on the outer container, which is separate from the Wrapper component... so inevitably someone someday is going to try to use the Wrapper component and be like "wHy DoEsN't ThIs WoRk?!!!" because they forgot to set `display: flex` and `flex-direction: column` on the outer container.
The alternative to both flexbox and float is what's used on the website: the Bubble component consists of a relatively positioned "container" with an absolutely positioned "bar" (what we see as the "bubble") that's translated sideways and cut off at the edge of the screen. This way, devs don't need to remember to do anything extra when using the component, because everything that's required for the component to work properly is an internal detail.
snedadah
commented
Yep, I did see the way it was done in the website, however since this technique is quite different than that I don't think it's worth it to restart using that approach, considering that the issue you outlined is not that likely that severe (only us using these components) and the scope of this project isn't really that big. Yep, I did see the way it was done in the website, however since this technique is quite different than that I don't think it's worth it to restart using that approach, considering that the issue you outlined is not that likely that severe (only us using these components) and the scope of this project isn't really that big.
|
||||
flex-direction: column;
|
||||
justify-content: center;
|
||||
}
|
|
@ -0,0 +1,127 @@
|
|||
import { BarGraphHorizontal, BarGraphVertical } from "components/BarGraph";
|
||||
import { mockCategoricalData, moreMockCategoricalData } from "data/mocks";
|
||||
import React from "react";
|
||||
import { useWindowDimensions } from "utils/getWindowDimensions";
|
||||
import { useIsMobile } from "utils/isMobile";
|
||||
|
||||
import { ComponentWrapper } from "@/components/ComponentWrapper";
|
||||
|
||||
import { WordCloud } from "../components/WordCloud";
|
||||
|
||||
import styles from "./samplePage.module.css";
|
||||
|
||||
export default function SamplePage() {
|
||||
const { width } = useWindowDimensions();
|
||||
const isMobile = useIsMobile();
|
||||
|
||||
return (
|
||||
<div className={styles.page}>
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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}
|
||||
// For components that are in the side wrappers, it looks better if they fill a certain amount of width, so we can make the width dynamic like this
|
||||
width={isMobile ? width / 1.25 : width / 2}
|
||||
height={500}
|
||||
margin={{
|
||||
top: 20,
|
||||
bottom: 80,
|
||||
left: 60,
|
||||
right: 20,
|
||||
}}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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."
|
||||
align="center"
|
||||
>
|
||||
<WordCloud
|
||||
data={moreMockCategoricalData.map((word) => ({
|
||||
text: word.key,
|
||||
value: word.value,
|
||||
}))}
|
||||
// For components that we don't want to match the width necessarily we can provide direct values
|
||||
width={isMobile ? width / 1.5 : 800}
|
||||
height={500}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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."
|
||||
align="right"
|
||||
>
|
||||
<BarGraphHorizontal
|
||||
className={styles.barGraphDemo}
|
||||
data={mockCategoricalData}
|
||||
width={isMobile ? width / 1.45 : width / 2}
|
||||
height={500}
|
||||
margin={{
|
||||
top: 20,
|
||||
bottom: 40,
|
||||
left: 150,
|
||||
right: 20,
|
||||
}}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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."
|
||||
align="left"
|
||||
noBackground
|
||||
>
|
||||
<BarGraphHorizontal
|
||||
className={styles.barGrapDemo}
|
||||
data={mockCategoricalData}
|
||||
width={isMobile ? width / 1.45 : width / 2}
|
||||
height={500}
|
||||
margin={{
|
||||
top: 20,
|
||||
bottom: 40,
|
||||
left: 150,
|
||||
right: 20,
|
||||
}}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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."
|
||||
>
|
||||
<BarGraphHorizontal
|
||||
className={styles.barGraphDemo}
|
||||
data={mockCategoricalData}
|
||||
width={isMobile ? width / 1.45 : width / 2}
|
||||
height={500}
|
||||
margin={{
|
||||
top: 20,
|
||||
bottom: 40,
|
||||
left: 150,
|
||||
right: 20,
|
||||
}}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
|
||||
<ComponentWrapper
|
||||
heading="What program are you in?"
|
||||
bodyText="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."
|
||||
align="left"
|
||||
noBackground
|
||||
>
|
||||
<WordCloud
|
||||
data={moreMockCategoricalData.map((word) => ({
|
||||
text: word.key,
|
||||
value: word.value,
|
||||
}))}
|
||||
width={isMobile ? width / 1.5 : width / 2}
|
||||
height={500}
|
||||
/>
|
||||
</ComponentWrapper>
|
||||
</div>
|
||||
);
|
||||
}
|
|
@ -0,0 +1,35 @@
|
|||
// Attribution: https://stackoverflow.com/questions/36862334/get-viewport-window-height-in-reactjs
|
||||
snedadah marked this conversation as resolved
a258wang
commented
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,
|
||||
};
|
||||
};
|
||||
|
||||
export 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;
|
||||
};
|
|
@ -0,0 +1,3 @@
|
|||
import { useWindowDimensions } from "./getWindowDimensions";
|
||||
|
||||
export const useIsMobile = () => useWindowDimensions().width <= 768;
|
||||
snedadah marked this conversation as resolved
Outdated
a258wang
commented
Could this be implemented using the Could this be implemented using the `useWindowDimension` hook from above?
a258wang
commented
NIT: Should this be NIT: Should this be `<` or `<=`?
|
This is a feature that we already have, its just the linter does not know it.