Header/Side Menu #52
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#52
Loading…
Reference in New Issue
No description provided.
Delete Branch "shahanneda/header"
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?
https://shahanneda-header-csc-class-profile-staging-snedadah.k8s.csclub.cloud/samplePage/
WIP: Header/Side Menuto Header/Side Menu@ -0,0 +1,144 @@
.headerWrapper {
display: flex;
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?
@ -0,0 +2,4 @@
display: flex;
justify-content: space-between;
align-items: center;
position: fixed;
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.@ -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);
NIT: can be simplified to
(top, left/right, bottom)
@ -0,0 +20,4 @@
position: fixed;
right: 0;
top: 0;
width: 20vw;
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.
@ -0,0 +33,4 @@
.sideBarShown {
composes: sideBarCommon;
transition: transform 0.8s;
transform: translateX(0%);
translateX(0%)
necessary? I think that once thetranslateX(100%)
style is removed, then the component will return to its original position.@ -0,0 +55,4 @@
.backgroundTintShow {
composes: backgroundTintCommon;
transition: opacity 0.8s;
similar to above, could transition be part of common class?
@ -0,0 +72,4 @@
margin-bottom: 0;
padding-left: calc(20rem / 16);
padding-bottom: 0;
font-size: cacl(50rem / 16);
typo,
calc
@ -0,0 +96,4 @@
@media screen and (max-width: 768px) {
.sideBarCommon {
width: 90vw;
Imo 90vw is too wide on mobile, could we maybe specify the exact width and/or a minimum/maximum width?
@ -0,0 +108,4 @@
}
.headerWrapper {
padding: calc(10rem / 16) calc(20rem / 16) 0 calc(20rem / 16);
NIT: can be simplified like above (top, left/right, bottom)
@ -0,0 +136,4 @@
display: flex;
}
.lineWrapper:before {
Is there an advantage to doing this instead of just using a div w/ specific width + background colour to draw the line? 😅
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" />
NIT: We should use the Next.js
Image
component instead of img.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} />
What is the purpose of this spacer?
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, usingposition: sticky
that is no longer needed :)@ -0,0 +53,4 @@
</div>
</div>
<div
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.
@ -0,0 +60,4 @@
: styles.backgroundTintHidden
}
onClick={(_) => {
setIsShowingMenu(false);
NIT: refactor into a
hideMenu
function instead of using an inline function, since this same function is used twice? (could do the same forshowMenu
as well)I really don't think this is necessary 😅
@ -0,0 +67,4 @@
);
}
const ListItem: React.FC<{ link: string; children: string }> = ({
NIT: remove this since it's not being used (not sure why the lint step in our pipeline isn't catching things like this)
Edited lint rules to stop this in the future.
Some visual nitpicks:
Can we align the "Sections" header with the section titles? The arrow button could probably be a little smaller if needed (particularly on mobile).
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?
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.
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.
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!
@ -84,2 +84,4 @@
const DEFAULT_LEGEND_GAP = 16;
// TODO: Address unused props in this file
/* eslint-disable unused-imports/no-unused-vars*/
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.
@ -19,1 +18,4 @@
plugins: ["@typescript-eslint", "react", "react-hooks", "prettier", "unused-imports"],
rules: {
"no-unused-vars": "off",
"unused-imports/no-unused-imports": "error",
NIT: some of these indents look a little big?
@ -0,0 +15,4 @@
margin: calc(16rem / 16) 0;
}
.sideBarCommon {
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.@ -0,0 +33,4 @@
.sideBarShown {
composes: sideBarCommon;
/* -1% to hide slight line tip showing in some browsers */
transform: translateX(-1%);
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. 😂