Header/Side Menu #52

Merged
snedadah merged 50 commits from shahanneda/header into main 2022-10-22 14:04:13 -04:00
Owner
https://shahanneda-header-csc-class-profile-staging-snedadah.k8s.csclub.cloud/samplePage/
snedadah added 36 commits 2022-09-14 00:19:22 -04:00
continuous-integration/drone/push Build is failing Details
d0dc94441d
Initial sections commit
continuous-integration/drone/push Build is passing Details
ff2ec0ce3b
Add Sections
d12261266e
Add Quotation Carousel (#36)
Made without visx, because that was easier.

Closes #10.

Co-authored-by: Amy Wang <a258wang@csclub.uwaterloo.ca>
Reviewed-on: #36
Reviewed-by: j285he <j285he@localhost>
continuous-integration/drone/push Build is failing Details
7c379f35c4
Added header button
continuous-integration/drone/push Build is failing Details
e2e6d1bb8b
Added tint to button
continuous-integration/drone/push Build is failing Details
7828090cef
Added fixed header
continuous-integration/drone/push Build is failing Details
7ca3d31630
Temp adjusted sections
continuous-integration/drone/push Build is failing Details
b30b459426
empty commit to rebuild
continuous-integration/drone/push Build is failing Details
5860eede84
Fixed lint
continuous-integration/drone/push Build is passing Details
b62d811e11
Changed image component
continuous-integration/drone/push Build is passing Details
0c513d9698
Added home page button
continuous-integration/drone/push Build is passing Details
9f77dc653b
Added mobile spacer
snedadah added 1 commit 2022-09-14 00:22:29 -04:00
continuous-integration/drone/push Build is failing Details
798a8a9b4f
Fixed issue with merge
snedadah added 1 commit 2022-09-14 00:35:58 -04:00
continuous-integration/drone/push Build is passing Details
382d51fa58
Fixed minor issues from merging
snedadah added 1 commit 2022-09-21 11:11:17 -04:00
continuous-integration/drone/push Build is passing Details
494b6030b8
Updated css
snedadah changed title from WIP: Header/Side Menu to Header/Side Menu 2022-09-21 11:11:37 -04:00
snedadah requested review from j285he 2022-09-21 11:26:05 -04:00
snedadah requested review from a258wang 2022-09-21 11:26:05 -04:00
snedadah requested review from b72zhou 2022-09-21 11:26:05 -04:00
snedadah requested review from e26chiu 2022-09-21 11:26:06 -04:00
a258wang removed review request for b72zhou 2022-09-29 01:22:50 -04:00
a258wang removed review request for e26chiu 2022-09-29 01:22:52 -04:00
a258wang requested changes 2022-10-02 14:24:21 -04:00
@ -0,0 +1,144 @@
.headerWrapper {
display: flex;
Owner

seems like we're using tabs instead of 2 spaces for the indents here, could we please change it to be more consistent with the rest of the project?

seems like we're using tabs instead of 2 spaces for the indents here, could we please change it to be more consistent with the rest of the project?
snedadah marked this conversation as resolved
@ -0,0 +2,4 @@
display: flex;
justify-content: space-between;
align-items: center;
position: fixed;
Owner

I would recomment using position: sticky instead. position: fixed removes the element from the normal document flow so that content can go behind it, which would mean that we'd need to manually add extra padding at the top of every page if we don't want things to get hidden behind the header.

I would recomment using `position: sticky` instead. `position: fixed` removes the element from the normal document flow so that content can go behind it, which would mean that we'd need to manually add extra padding at the top of every page if we don't want things to get hidden behind the header.
snedadah marked this conversation as resolved
@ -0,0 +9,4 @@
background: var(--dark--primary-background);
z-index: 98;
box-sizing: border-box;
padding: calc(10rem / 16) calc(100rem / 16) 0 calc(100rem / 16);
Owner

NIT: can be simplified to

padding: calc(10rem / 16) calc(100rem / 16) 0;

(top, left/right, bottom)

NIT: can be simplified to ```css padding: calc(10rem / 16) calc(100rem / 16) 0; ``` (top, left/right, bottom)
snedadah marked this conversation as resolved
@ -0,0 +20,4 @@
position: fixed;
right: 0;
top: 0;
width: 20vw;
Owner

Can we please specify a min/max width as well? Or since this style only applies to screen widths > 768px, we could just specify the exact width for the side bar. 20vw on a 800px wide screen is a little small, imo.

Can we please specify a min/max width as well? Or since this style only applies to screen widths > 768px, we could just specify the exact width for the side bar. 20vw on a 800px wide screen is a little small, imo.
snedadah marked this conversation as resolved
@ -0,0 +33,4 @@
.sideBarShown {
composes: sideBarCommon;
transition: transform 0.8s;
transform: translateX(0%);
Owner
  1. could the transition be part of the common class?
  2. is translateX(0%) necessary? I think that once the translateX(100%) style is removed, then the component will return to its original position.
1. could the transition be part of the common class? 2. is `translateX(0%)` necessary? I think that once the `translateX(100%)` style is removed, then the component will return to its original position.
snedadah marked this conversation as resolved
@ -0,0 +55,4 @@
.backgroundTintShow {
composes: backgroundTintCommon;
transition: opacity 0.8s;
Owner

similar to above, could transition be part of common class?

similar to above, could transition be part of common class?
snedadah marked this conversation as resolved
@ -0,0 +72,4 @@
margin-bottom: 0;
padding-left: calc(20rem / 16);
padding-bottom: 0;
font-size: cacl(50rem / 16);
Owner

typo, calc

typo, `calc`
snedadah marked this conversation as resolved
@ -0,0 +96,4 @@
@media screen and (max-width: 768px) {
.sideBarCommon {
width: 90vw;
Owner

Imo 90vw is too wide on mobile, could we maybe specify the exact width and/or a minimum/maximum width?

Imo 90vw is too wide on mobile, could we maybe specify the exact width and/or a minimum/maximum width?
snedadah marked this conversation as resolved
@ -0,0 +108,4 @@
}
.headerWrapper {
padding: calc(10rem / 16) calc(20rem / 16) 0 calc(20rem / 16);
Owner

NIT: can be simplified like above (top, left/right, bottom)

NIT: can be simplified like above (top, left/right, bottom)
snedadah marked this conversation as resolved
@ -0,0 +136,4 @@
display: flex;
}
.lineWrapper:before {
Owner

Is there an advantage to doing this instead of just using a div w/ specific width + background colour to draw the line? 😅

Is there an advantage to doing this instead of just using a div w/ specific width + background colour to draw the line? 😅
Author
Owner

I don't recommended exactly why I did it this way, but I think I was having some trouble aligning things before, and a stack overflow post recommended doing it this way. Not sure if there are any advantages. But I also don't think there are any disadvantages.

I don't recommended exactly why I did it this way, but I think I was having some trouble aligning things before, and a stack overflow post recommended doing it this way. Not sure if there are any advantages. But I also don't think there are any disadvantages.
@ -0,0 +22,4 @@
}}
className={styles.menuIcon}
>
<img src="/images/menuIcon.svg" width="50" draggable="false" />
Owner

NIT: We should use the Next.js Image component instead of img.

NIT: We should use the Next.js `Image` component instead of img.
Author
Owner

AFAIK the advantage of Next Image is that it can optimize an image, however, this icon is small enough that it does not need to be optimized.

I did try it out with Next image. I had to change the image loader to a custom loader in order to work with next export.

However, the next image preforms a lot worse than normal images, when loading the page, next does some optimizing of next images, which takes around 0.2 seconds for the image to show, while the normal html tag is instant, thus I think we should stick to normal img tags for an icon which is small like this.

However, I left in the custom loader so we can use the Next image for other bigger images that might need optimizing.

https://stackoverflow.com/questions/65487914/error-image-optimization-using-next-js-default-loader-is-not-compatible-with-n

AFAIK the advantage of Next Image is that it can optimize an image, however, this icon is small enough that it does not need to be optimized. I did try it out with Next image. I had to change the image loader to a custom loader in order to work with `next export`. However, the next image preforms a lot worse than normal images, when loading the page, next does some optimizing of next images, which takes around 0.2 seconds for the image to show, while the normal html tag is instant, thus I think we should stick to normal img tags for an icon which is small like this. However, I left in the custom loader so we can use the Next image for other bigger images that might need optimizing. https://stackoverflow.com/questions/65487914/error-image-optimization-using-next-js-default-loader-is-not-compatible-with-n
@ -0,0 +26,4 @@
</button>
</div>
<div className={styles.mobileSpacer} />
Owner

What is the purpose of this spacer?

What is the purpose of this spacer?
Author
Owner

The reason was because I was using position: fixed, I needed to move the content down on mobile so it doesn't overlap with the menu, however, using position: sticky that is no longer needed :)

The reason was because I was using `position: fixed`, I needed to move the content down on mobile so it doesn't overlap with the menu, however, using `position: sticky` that is no longer needed :)
a258wang marked this conversation as resolved
@ -0,0 +53,4 @@
</div>
</div>
<div
Owner

NIT: reorder the code to put the background tint div before the sidebar menu div - even though the z-indexes ensure things appear in the correct order, normally elements that come later in the DOM are drawn on top of elements that come earlier so that would makes things feel better imo lol.

NIT: reorder the code to put the background tint div before the sidebar menu div - even though the z-indexes ensure things appear in the correct order, normally elements that come later in the DOM are drawn on top of elements that come earlier so that would makes things *feel* better imo lol.
snedadah marked this conversation as resolved
@ -0,0 +60,4 @@
: styles.backgroundTintHidden
}
onClick={(_) => {
setIsShowingMenu(false);
Owner

NIT: refactor into a hideMenu function instead of using an inline function, since this same function is used twice? (could do the same for showMenu as well)

NIT: refactor into a `hideMenu` function instead of using an inline function, since this same function is used twice? (could do the same for `showMenu` as well)
Author
Owner

I really don't think this is necessary 😅

I really don't think this is necessary 😅
@ -0,0 +67,4 @@
);
}
const ListItem: React.FC<{ link: string; children: string }> = ({
Owner

NIT: remove this since it's not being used (not sure why the lint step in our pipeline isn't catching things like this)

NIT: remove this since it's not being used (not sure why the lint step in our pipeline isn't catching things like this)
Author
Owner

Edited lint rules to stop this in the future.

Edited lint rules to stop this in the future.
a258wang marked this conversation as resolved
Owner

Some visual nitpicks:

  1. Can we align the "Sections" header with the section titles? The arrow button could probably be a little smaller if needed (particularly on mobile).
    image

  2. Not sure if it's just me but I'm getting a partial gray border on the arrow button (see image above), do you know why that's happening and/or how to fix it?

  3. I find it interesting that the arrow is brown, since this is the only place on the entire site that we use this colour LOL. I don't have any recommendations for alternatives though (other than getting rid of the arrow entirely) so I guess we can keep it as is for now.

  4. RE: the white line of the sidebar sometimes sticking out a little bit even when the side menu is hidden - maybe try setting the x-overflow to be clipped or something to prevent this? Not a big deal though, just an idea.
    image

Some visual nitpicks: 1. Can we align the "Sections" header with the section titles? The arrow button could probably be a little smaller if needed (particularly on mobile). ![image](/attachments/ec94c5fc-b323-45a9-8382-037325d868db) 2. Not sure if it's just me but I'm getting a partial gray border on the arrow button (see image above), do you know why that's happening and/or how to fix it? 3. I find it interesting that the arrow is brown, since this is the only place on the entire site that we use this colour LOL. I don't have any recommendations for alternatives though (other than getting rid of the arrow entirely) so I guess we can keep it as is for now. 4. RE: the white line of the sidebar sometimes sticking out a little bit even when the side menu is hidden - maybe try setting the x-overflow to be clipped or something to prevent this? Not a big deal though, just an idea. ![image](/attachments/0421899d-6046-46fc-a390-90afcdd8ffc6)
snedadah added 1 commit 2022-10-14 16:42:57 -04:00
continuous-integration/drone/push Build is passing Details
493d945101
Addressed pr comments
Author
Owner

Some visual nitpicks:

  1. Can we align the "Sections" header with the section titles? The arrow button could probably be a little smaller if needed (particularly on mobile).
    image

  2. Not sure if it's just me but I'm getting a partial gray border on the arrow button (see image above), do you know why that's happening and/or how to fix it?

  3. I find it interesting that the arrow is brown, since this is the only place on the entire site that we use this colour LOL. I don't have any recommendations for alternatives though (other than getting rid of the arrow entirely) so I guess we can keep it as is for now.

  4. RE: the white line of the sidebar sometimes sticking out a little bit even when the side menu is hidden - maybe try setting the x-overflow to be clipped or something to prevent this? Not a big deal though, just an idea.
    image

I was able to fix the weird border by adding a transparent border to the button.

I can't recreate the white bit sticking out on my end, but I just changed the tranform to -1% so hopefully that should fix it!

> Some visual nitpicks: > > 1. Can we align the "Sections" header with the section titles? The arrow button could probably be a little smaller if needed (particularly on mobile). > ![image](/attachments/ec94c5fc-b323-45a9-8382-037325d868db) > > 2. Not sure if it's just me but I'm getting a partial gray border on the arrow button (see image above), do you know why that's happening and/or how to fix it? > > 3. I find it interesting that the arrow is brown, since this is the only place on the entire site that we use this colour LOL. I don't have any recommendations for alternatives though (other than getting rid of the arrow entirely) so I guess we can keep it as is for now. > > 4. RE: the white line of the sidebar sometimes sticking out a little bit even when the side menu is hidden - maybe try setting the x-overflow to be clipped or something to prevent this? Not a big deal though, just an idea. > ![image](/attachments/0421899d-6046-46fc-a390-90afcdd8ffc6) I was able to fix the weird border by adding a transparent border to the button. I can't recreate the white bit sticking out on my end, but I just changed the tranform to -1% so hopefully that should fix it!
snedadah added 1 commit 2022-10-14 17:02:57 -04:00
continuous-integration/drone/push Build is failing Details
99c56122d8
Fixed visual nitpicks
snedadah added 1 commit 2022-10-14 17:07:08 -04:00
snedadah added 1 commit 2022-10-14 17:10:29 -04:00
continuous-integration/drone/push Build is passing Details
8b105abe02
Fixed next export command
snedadah reviewed 2022-10-14 17:13:24 -04:00
@ -84,2 +84,4 @@
const DEFAULT_LEGEND_GAP = 16;
// TODO: Address unused props in this file
/* eslint-disable unused-imports/no-unused-vars*/
Author
Owner

This file has lots of unused props, by adding the lint rule for unused vars, this file broke. Ignoring for now, I didn't just want to delete all the unused props since I want to discuss with the whomever wrote the file in case they plan on implmenting them later.

This file has lots of unused props, by adding the lint rule for unused vars, this file broke. Ignoring for now, I didn't just want to delete all the unused props since I want to discuss with the whomever wrote the file in case they plan on implmenting them later.
snedadah added 1 commit 2022-10-14 17:25:59 -04:00
continuous-integration/drone/push Build is passing Details
d58210b0c3
Fixed next image loader
snedadah added 1 commit 2022-10-14 17:34:15 -04:00
continuous-integration/drone/push Build is passing Details
f6e6aafee7
Updated image loader
snedadah added 2 commits 2022-10-14 17:54:06 -04:00
snedadah requested review from a258wang 2022-10-14 18:02:07 -04:00
snedadah added 1 commit 2022-10-14 18:05:16 -04:00
continuous-integration/drone/push Build is passing Details
f9cffdb0b4
Fixed horizontal scrollbar
a258wang approved these changes 2022-10-19 22:48:25 -04:00
.eslintrc.js Outdated
@ -19,1 +18,4 @@
plugins: ["@typescript-eslint", "react", "react-hooks", "prettier", "unused-imports"],
rules: {
"no-unused-vars": "off",
"unused-imports/no-unused-imports": "error",
Owner

NIT: some of these indents look a little big?

NIT: some of these indents look a little big?
snedadah marked this conversation as resolved
@ -0,0 +15,4 @@
margin: calc(16rem / 16) 0;
}
.sideBarCommon {
Owner

We should probably add overflow: auto or something similar so that if the menu options don't all fit on the screen, the user can scroll to see them.

We should probably add `overflow: auto` or something similar so that if the menu options don't all fit on the screen, the user can scroll to see them.
snedadah marked this conversation as resolved
@ -0,0 +33,4 @@
.sideBarShown {
composes: sideBarCommon;
/* -1% to hide slight line tip showing in some browsers */
transform: translateX(-1%);
Owner

Tbh idk if this is even necessary lol, it was such a small and specific nitpick in the first place. But I guess it doesn't hurt. 😂

Tbh idk if this is even necessary lol, it was such a small and specific nitpick in the first place. But I guess it doesn't hurt. 😂
snedadah marked this conversation as resolved
snedadah added 1 commit 2022-10-20 02:11:30 -04:00
continuous-integration/drone/push Build is passing Details
c2fec4f21f
Added scrolling to header
snedadah added 1 commit 2022-10-20 11:49:21 -04:00
continuous-integration/drone/push Build is passing Details
41c34bee65
updated routs
snedadah merged commit bfa3e9960c into main 2022-10-22 14:04:13 -04:00
snedadah deleted branch shahanneda/header 2022-10-22 14:04:14 -04:00
snedadah referenced this issue from a commit 2022-10-22 14:04:15 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#52
No description provided.