Sample page and graph wrappers #32

Merged
snedadah merged 18 commits from shahanneda/wrapper into main 5 months ago
Owner

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

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
snedadah added 8 commits 6 months ago
ac446f7b07
Created componet wrapper
8a85e3c5d9
Worked on FullComponentWrapper and graph widths
4755793d18
Made page responsive
7efe878e9e
Adjusted margins
3e19501003
Removed log
d3ceb38fe6
Typo
f21aa3aada
Cleaned up
41ec2654ee
Fixed typo
snedadah reviewed 6 months ago
"typescript.tsdk": "node_modules/typescript/lib",
"eslint.format.enable": true,
"eslint.codeActionsOnSave.mode": "all",
"css.lint.validProperties": ["composes"],
Poster
Owner

This is a feature that we already have, its just the linter does not know it.

This is a feature that we already have, its just the linter does not know it.
a258wang marked this conversation as resolved
snedadah reviewed 6 months ago
return <Component {...pageProps} />;
return (
<>
<Head>
Poster
Owner

For making all pages responsive

For making all pages responsive
Owner

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

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

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
Owner

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?
Poster
Owner

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)
Owner

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.
snedadah added 1 commit 6 months ago
d455d3a92e
Fixed wordcloud responsivness
snedadah requested review from j285he 6 months ago
snedadah requested review from a258wang 6 months ago
snedadah requested review from b72zhou 6 months ago
snedadah requested review from e26chiu 6 months ago
a258wang reviewed 6 months ago
a258wang left a comment
Owner

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

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. 🙂
display: flex;
flex-direction: column;
text-align: center;
width: 80%;
Owner

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.
snedadah marked this conversation as resolved
.graphWrapper {
margin-top: calc(50rem / 16);
Owner

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
Poster
Owner

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.
a258wang marked this conversation as resolved
body: string;
};
export default function FullComponentWrapper({
Owner

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"?
snedadah marked this conversation as resolved
children,
}: FullComponentWrapperProps) {
return (
<div className={styles.wrapper}>
Owner

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`.)
Poster
Owner

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

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. 🙂
snedadah marked this conversation as resolved
<div className={styles.wrapper}>
<div className={styles.textWrapper}>
<h3 className={styles.heading}>{heading}</h3>
<p className={styles.body}>{body}</p>
Owner

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
snedadah marked this conversation as resolved
.wrapperCommon {
background-color: var(--secondary-background);
display: flex;
width: 95%;
Owner

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.
Poster
Owner

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.
children: React.ReactNode;
heading: string;
body: string;
rightAligned?: boolean;
Owner

NIT: could do align: "left" | "right" just to be explicit

NIT: could do `align: "left" | "right"` just to be explicit
Owner

In 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)

In 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)
Poster
Owner

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.

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.
a258wang marked this conversation as resolved
rightAligned?: boolean;
};
export default function SideComponentWrapper({
Owner

Usually we make our components non-default exports

Usually we make our components non-default exports
snedadah marked this conversation as resolved
return (
<div className={rightAligned ? styles.wrapperRight : styles.wrapperLeft}>
{rightAligned && !isMobile ? (
Owner

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?
Poster
Owner

Great point! totally forgot those were a thing

Great point! totally forgot those were a thing
a258wang marked this conversation as resolved
package.json Outdated
"react": "18.1.0",
"react-dom": "18.1.0"
"react-dom": "18.1.0",
"react-responsive": "^9.0.0-beta.10"
Owner

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:

  1. Is it really necessary? (Can we accomplish the same functionality using CSS, or code it easily ourselves?)
  2. How popular and well-maintained is the package? (We don't want to find ourselves depending on something that is no longer maintained.)

In this case:

  1. We have a fairly simple useWindowDimension hook in the website that we could copy over here EDIT: I just saw that you already added the same useWindowDimensions hook 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.
  2. react-responsive has 520k weekly downloads on npm and 6.3k stars on GitHub, which is reasonably popular.

I'll leave the final decision up to you. 🙂

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: 1. Is it really necessary? (Can we accomplish the same functionality using CSS, or code it easily ourselves?) 2. How popular and well-maintained is the package? (We don't want to find ourselves depending on something that is no longer maintained.) In this case: 1. ~~We have a fairly simple [useWindowDimension hook](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/hooks/useWindowDimension.tsx) in the website that we could copy over here~~ EDIT: I just saw that you already added the same `useWindowDimensions` hook 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. 2. react-responsive has 520k weekly downloads on npm and 6.3k stars on GitHub, which is reasonably popular. I'll leave the final decision up to you. 🙂
snedadah marked this conversation as resolved
}
p {
font-size: calc(28rem / 16);
Owner

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
Poster
Owner

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.
a258wang marked this conversation as resolved
pages/index.tsx Outdated
return (
<p>
Click <Link href="/playground">here</Link> to visit the playground
<br></br>
Owner

NIT: <br /> 😆

NIT: `<br />` 😆
snedadah marked this conversation as resolved
.page {
display: flex;
Owner

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)

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`)
Poster
Owner

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

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.

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.
Poster
Owner

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.
// Attribution: https://stackoverflow.com/questions/36862334/get-viewport-window-height-in-reactjs
Owner

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 😂
snedadah marked this conversation as resolved
return windowDimensions;
};
export default useWindowDimensions;
Owner

NIT: make this non-default

NIT: make this non-default
snedadah marked this conversation as resolved
import { useMediaQuery } from "react-responsive";
export const useIsMobile = () => useMediaQuery({ query: "(max-width: 768px)" });
Owner

Could this be implemented using the useWindowDimension hook from above?

Could this be implemented using the `useWindowDimension` hook from above?
snedadah marked this conversation as resolved
snedadah added 1 commit 6 months ago
29534e574b
Worked on no background + pr comments
snedadah added 1 commit 6 months ago
ce302f6c03
Change align prop
snedadah added 1 commit 6 months ago
5bca794f85
Cleaned up
snedadah added 1 commit 6 months ago
216dece1ed
Updated styling
snedadah added 1 commit 6 months ago
d44f79ac43
Merged center and side wrappers
snedadah added 1 commit 6 months ago
ce9480aeee
added comment
snedadah added 1 commit 6 months ago
cf817b6239
Updated comment
a258wang reviewed 6 months ago
.wrapperCommon {
Owner

NIT: maybe rename this to something like sideWrapperCommon or just sideWrapper, since it's only common to the left and right wrappers?

NIT: maybe rename this to something like `sideWrapperCommon` or just `sideWrapper`, since it's only common to the left and right wrappers?
snedadah marked this conversation as resolved
border-radius: 0;
width: 100%;
}
Owner

NIT: unnecessary empty line :>

NIT: unnecessary empty line :>
snedadah marked this conversation as resolved
type ComponentWrapperProps = {
children: React.ReactNode;
heading: string;
body: string;
Owner

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 🤔

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` 🤔
Poster
Owner

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.
snedadah marked this conversation as resolved
body: string;
align?: AlignOption;
noBackground?: boolean;
center?: boolean;
Owner

unused prop

unused prop
snedadah marked this conversation as resolved
import { useWindowDimensions } from "./getWindowDimensions";
export const useIsMobile = () => useWindowDimensions().width < 768;
Owner

NIT: Should this be < or <=?

NIT: Should this be `<` or `<=`?
snedadah marked this conversation as resolved
snedadah added 1 commit 5 months ago
0f514d974c
Addressed PR comments
snedadah added 1 commit 5 months ago
f6dcda1c72
Fixed mobile wrapper
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

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

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. 🙂
snedadah merged commit 9cd5c158e7 into main 5 months ago
snedadah deleted branch shahanneda/wrapper 5 months ago

Reviewers

j285he was requested for review 6 months ago
b72zhou was requested for review 6 months ago
e26chiu was requested for review 6 months ago
a258wang approved these changes 5 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 9cd5c158e7.
Sign in to join this conversation.
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…
There is no content yet.