Header/Side Menu #52

Merged
snedadah merged 50 commits from shahanneda/header into main 1 month ago
Owner
https://shahanneda-header-csc-class-profile-staging-snedadah.k8s.csclub.cloud/samplePage/
snedadah added 36 commits 2 months ago
d0dc94441d Initial sections commit
ff2ec0ce3b Add Sections
d12261266e
Add Quotation Carousel (#36)
7c379f35c4
Added header button
e2e6d1bb8b
Added tint to button
7828090cef
Added fixed header
7ca3d31630
Temp adjusted sections
b30b459426
empty commit to rebuild
5860eede84
Fixed lint
b62d811e11
Changed image component
0c513d9698
Added home page button
9f77dc653b
Added mobile spacer
snedadah added 1 commit 2 months ago
798a8a9b4f
Fixed issue with merge
snedadah added 1 commit 2 months ago
382d51fa58
Fixed minor issues from merging
snedadah added 1 commit 2 months ago
494b6030b8
Updated css
snedadah changed title from WIP: Header/Side Menu to Header/Side Menu 2 months ago
snedadah requested review from j285he 2 months ago
snedadah requested review from a258wang 2 months ago
snedadah requested review from b72zhou 2 months ago
snedadah requested review from e26chiu 2 months ago
a258wang removed review request for b72zhou 2 months ago
a258wang removed review request for e26chiu 2 months ago
a258wang requested changes 2 months ago
.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
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
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
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
.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
.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
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
@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
}
.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
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? 😅
Poster
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.
}}
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.
Poster
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
</button>
</div>
<div className={styles.mobileSpacer} />
Owner

What is the purpose of this spacer?

What is the purpose of this spacer?
Poster
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
</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
: 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)
Poster
Owner

I really don't think this is necessary 😅

I really don't think this is necessary 😅
);
}
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)
Poster
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 1 month ago
493d945101 Addressed pr comments
Poster
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 1 month ago
99c56122d8 Fixed visual nitpicks
snedadah added 1 commit 1 month ago
snedadah added 1 commit 1 month ago
8b105abe02 Fixed next export command
snedadah reviewed 1 month ago
const DEFAULT_LEGEND_GAP = 16;
// TODO: Address unused props in this file
/* eslint-disable unused-imports/no-unused-vars*/
Poster
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 1 month ago
d58210b0c3 Fixed next image loader
snedadah added 1 commit 1 month ago
f6e6aafee7 Updated image loader
snedadah added 2 commits 1 month ago
snedadah requested review from a258wang 1 month ago
snedadah added 1 commit 1 month ago
f9cffdb0b4 Fixed horizontal scrollbar
a258wang approved these changes 1 month ago
.eslintrc.js Outdated
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
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
.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 1 month ago
c2fec4f21f Added scrolling to header
snedadah added 1 commit 1 month ago
41c34bee65 updated routs
snedadah merged commit bfa3e9960c into main 1 month ago
snedadah deleted branch shahanneda/header 1 month ago
snedadah referenced this issue from a commit 1 month ago

Reviewers

j285he was requested for review 2 months ago
a258wang approved these changes 1 month ago
continuous-integration/drone/push Build is passing
The pull request has been merged as bfa3e9960c.
Sign in to join this conversation.
No reviewers
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#52
Loading…
There is no content yet.