Sample page and graph wrappers #32
j285he
b72zhou
e26chiu
a258wang
Labels
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: www/cs-2022-class-profile#32
Loading…
Reference in New Issue
No description provided.
Delete Branch "shahanneda/wrapper"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Added a sample page, and a component wrapper for aiding in displaying the pages.
I made the sample page graphs responsive by adjusting the width of the graphs according to the width of the page, I think this may be the best way to go, but let me know if you have any other ideas.
The word cloud (and general margins) is still a bit buggy on mobile . I'll work on that separately after merging this.
https://shahanneda-wrapper-csc-class-profile-staging-snedadah.k8s.csclub.cloud
Closes #29
@ -2,12 +2,13 @@"typescript.tsdk": "node_modules/typescript/lib","eslint.format.enable": true,"eslint.codeActionsOnSave.mode": "all","css.lint.validProperties": ["composes"],This is a feature that we already have, its just the linter does not know it.
@ -8,1 +9,3 @@return <Component {...pageProps} />;return (<><Head>For making all pages responsive
Is this required? (I'm guessing it's needed for 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
Today I learned, LOL. Though it doesn't seem like we have this setup for the website and the responsive shenanigans work fine?
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)
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.
Thanks for taking initiative and working on this, Shahan! I think the overall structure and interface of the component wrappers is pretty good, just left some nitpicky comments about implementation details. 🙂
@ -0,0 +4,4 @@display: flex;flex-direction: column;text-align: center;width: 80%;Instead of specifying the width as a percentage, what if we tried
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.@ -0,0 +12,4 @@.graphWrapper {margin-top: calc(50rem / 16);NIT: could use
gap(and maybespace-between?) on the flexbox instead of margin hereI 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.
@ -0,0 +8,4 @@body: string;};export default function FullComponentWrapper({Our convention is usually to make component exports non-default.
Also, NIT: would it make more sense to call this
CenterComponentWrapperor something involving the word "center"?@ -0,0 +14,4 @@children,}: FullComponentWrapperProps) {return (<div className={styles.wrapper}>Would it make sense to use a
sectioninstead of adiv? (If so, it'd also probably make sense to just use anh1inside thesection.)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?
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. 🙂
@ -0,0 +17,4 @@<div className={styles.wrapper}><div className={styles.textWrapper}><h3 className={styles.heading}>{heading}</h3><p className={styles.body}>{body}</p>These classes don't appear to be declared in the CSS module, please remove them if they're not needed
@ -0,0 +1,38 @@.wrapperCommon {background-color: var(--secondary-background);display: flex;width: 95%;I would suggest taking a look at the
Bubblecomponent 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.
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.
@ -0,0 +7,4 @@children: React.ReactNode;heading: string;body: string;rightAligned?: boolean;NIT: could do
align: "left" | "right"just to be explicitIn fact, this could potentially be extended to
align: "left" | "center" | "right"and then we just have one component wrapper with different styles 👀(Just an idea, separating between side vs. center wrapper is fine for now)
I don't think we should merge the components, the css would be unnecessarily more complex, since the center component is quite differnet than these ones.
edit: It was easier than I once thought 😅 , I ended up merging them.
@ -0,0 +10,4 @@rightAligned?: boolean;};export default function SideComponentWrapper({Usually we make our components non-default exports
@ -0,0 +20,4 @@return (<div className={rightAligned ? styles.wrapperRight : styles.wrapperLeft}>{rightAligned && !isMobile ? (Would it work if we set
flex-direction: rowvsflex-direction: row-reversefor the left/right wrapper classes, instead of having this awkward conditional thing here?Great point! totally forgot those were a thing
@ -28,2 +28,3 @@"react": "18.1.0","react-dom": "18.1.0""react-dom": "18.1.0","react-responsive": "^9.0.0-beta.10"I'm not entirely opposed to adding a new package for this purpose, but I just want to make sure we're thinking carefully when adding dependencies in general:
In this case:
We have a fairly simple useWindowDimension hook in the website that we could copy over hereEDIT: I just saw that you already added the sameuseWindowDimensionshook here. Then we can either either throw a wrapping useIsMobile thing around it, or just use the exact window width/height for more precise calculations.I'll leave the final decision up to you. 🙂
@ -90,2 +94,4 @@}p {font-size: calc(28rem / 16);This font is so big 😂 Let's create an issue to standardize the font sizes to be reasonable across the site
Edit: #33
Leaving it as it is for now, we can handle it later in that issue.
@ -5,6 +5,8 @@ export default function Home() {return (<p>Click <Link href="/playground">here</Link> to visit the playground<br></br>NIT:
<br />😆@ -0,0 +1,4 @@.page {display: flex;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)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.
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: flexandflex-direction: columnon 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.
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.
@ -0,0 +1,37 @@// Attribution: https://stackoverflow.com/questions/36862334/get-viewport-window-height-in-reactjsI'm pretty sure we referenced the same thing when working on the website, but we definitely didn't cite our sources like this 😂
@ -0,0 +34,4 @@return windowDimensions;};export default useWindowDimensions;NIT: make this non-default
@ -0,0 +1,3 @@import { useMediaQuery } from "react-responsive";export const useIsMobile = () => useMediaQuery({ query: "(max-width: 768px)" });Could this be implemented using the
useWindowDimensionhook from above?@ -0,0 +1,60 @@.wrapperCommon {NIT: maybe rename this to something like
sideWrapperCommonor justsideWrapper, since it's only common to the left and right wrappers?@ -0,0 +52,4 @@border-radius: 0;width: 100%;}NIT: unnecessary empty line :>
@ -0,0 +7,4 @@type ComponentWrapperProps = {children: React.ReactNode;heading: string;body: string;NIT: maybe rename to
bodyTextor justtext? 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
GraphWrapperinstead of justComponentWrapper🤔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.
@ -0,0 +10,4 @@body: string;align?: AlignOption;noBackground?: boolean;center?: boolean;unused prop
@ -0,0 +1,3 @@import { useWindowDimensions } from "./getWindowDimensions";export const useIsMobile = () => useWindowDimensions().width < 768;NIT: Should this be
<or<=?I'm still iffy about requiring the outside container to be a flexbox, but I agree that it's not worth refactoring that right now.
So let's merge this for now, and tweak things later if needed. 🙂