Tablet Navbar issues fixed#492
Conversation
mahigangal
changed the title
In regards to this, do you think you can shorten the width of the navbar so there isn't a whole lot of empty space? Probably just by adding a max-width.
Also, the nav elements have the blue underline in that screenshot and are aligned to the right, would it be possible to try to match the styling of mobile? |
AdamFipke
left a comment
AdamFipke
left a comment
There was a problem hiding this comment.
See comments.
Rendering 3 navbars and using 2 of them just for measuring the width of the desktop navbar and condensed navbar to determine what mode the true navbar should be in is a creative approach, but I'm unsure if it'll be worth the performance cost. I will leave this to you to test more. Maybe one of us can come up with another way that's better for performance if it turns out to be noticeably hurting performance.
Check out React Dev Tools and see if there's lots of unnecessary re-renders. Also try using chrome dev tools to throttle performance and see if it stills feels responsive.
| const showDesktopPresentation = | ||
| orientation === 'horizontal' && !forceDrawerPresentation | ||
| const showDrawerPresentation = | ||
| orientation === 'vertical' || forceDrawerPresentation |
There was a problem hiding this comment.
Small thing. As much as this logic is fine in that if one is false the other is true, I would rather just have 1 variable for maintainability reasons (if in the future these are changed, it might accidentally create a state where both showDesktopPresentation and showDrawerPresentation are both true or both false).
| const [isDrawerOpen, setIsDrawerOpen] = useState(false) | ||
| const isDesktop = useMediaQuery('(min-width: 768px)') | ||
| const [navMode, setNavMode] = useState<NavMode>('drawer') | ||
| const isPhone = useMediaQuery('(max-width: 640px)') |
There was a problem hiding this comment.
you might want to leave this as 768px instead of 640px since basically the entire site uses md: as the breakpoint between mobile and desktop for CSS, so in the case where the browser width is at 700px it might show the desktop navbar but with mobile styles.
| return ( | ||
| <div ref={availableWidthRef} className="relative w-full"> | ||
| <div className="pointer-events-none invisible absolute left-0 top-0 -z-10 h-0 overflow-hidden"> | ||
| <div ref={regularMeasureRef} className="w-max"> |
There was a problem hiding this comment.
Okay I get this now.
We're rendering 3 navbar components all the time.
2 of these navbar components are invisible and literally only exist so their width can be measured. 1 navbar has the condensed classnames, and the other the standard desktop navbar.
The final navbar is the real one.
It's kinda creative, but I think rendering 3 navbars might have some performance cost considerations that might not be worth itt so I might wait on merging this in case someone thinks of a better solution.
Also the 2 invisible navbars wouldn't technically be the true width since the CSS styles may adjust the width of the navbar (and its items) once it hits below the mobile breakpoint. But I guess as long as you have that isPhone like you have that matches the breakpoint of the CSS, that should be fine since it'll always ensure the mode is in drawer below that breakpoint regardless of if the CSS styles shrink the invisible navbars. I would really put this in a comment though
|
|
||
| useLayoutEffect(() => { | ||
| updateNavMode() | ||
| }, [updateNavMode, pathname, course, userInfo]) |
There was a problem hiding this comment.
For someone fresh looking at this, I would definitely put a comment saying what this code is doing.
"This is to update the navmode when the navbar would get different elements. For example, there's different navbar elements from organization view vs course view, professor view vs TA view vs student view, based on whether the course has queues enabled, etc. useLayoutEffect is used instead of useEffect so that setNavMode calculations are done before the browser repaints the screen (so users don't see an incorrect navbar for a frame)"
I will note:
- you will want to use useCourseFeatures since that's where stuff like isQueuesEnabled are.
- Debounce, debounce, debounce. If an object is changing a lot, each one will trigger a re-render for this. For the use effect below, you REALLY want to debounce it since if the browser width is changing a lot, it will spam a ton of re-renders.
- Instead of having a useEffect/useLayoutEffect with some seemingly arbitrary dependency array variables (which will throw a linter error btw if you have the react extension), you would instead want to take advantage of the fact that there are invisible navbars being rendered and use their widths. I notice that the useEffect hook below does use the width of the invisible navbars. So, you should use that logic inside of a useLayoutEffect and just remove the arbitrary array dependencies. I also want to be clear that I'm still not totally on board with the idea of rendering 3 navbars, but if we decide to go that way, we should remove the redundant code that would be causing extra re-renders as that's kinda overkill and causing unnecessary performance loss. I would also double check to make sure that it still works after making these changes. If it doesn't, then there is a flaw in the approach of using invisible navbars and measuring their widths and another approach should be used.
- Get the React Developer Tools extension for chrome and turn on "Highlight updates when components render" (under browser inspect -> Components -> Settings Cog -> General). Try resizing the window and see how many times the navbar re-renders. Also try navigating around a course and see how many times it re-renders.
There was a problem hiding this comment.
Huh, so I went and manually tested it while going to try something else and maybe this is less performance intensive than I thought? Like it really does only trigger the re-render at the breakpoints. Moving the window width a lot doesn't seem to cause any re-renders. So maybe debouncing isn't really that necessary.
Although I guess this only shows the visible navbar. The invisible ones would also be re-rendering a bunch.
There was a problem hiding this comment.
Okay I was correct about the dependency arrays though (and that there was a useEffect and a useLayoutEffect when a useLayoutEffect was needed).
There was a problem hiding this comment.
Okay I did find one kinda funny bug with it that causes an infinite loop, but it needs super specific circumstances:
- Shrink to drawer view. Open the drawer (but don't close it)
- Expand the viewport slowly to the point where it becomes the desktop view.
This will close the drawer. When the drawer closes, the viewport width shrinks a tiny bit (it's a feature from that drawer component), this causes a switch from 'desktop' view to 'drawer' view since it's just below the breakpoint width again. This re-opens the drawer (since it never fully closed), expanding the viewport width a tiny bit, switching to 'desktop' mode again, which then closes the drawer, causing the width to shrink, which re-opens the drawer again, and so on. It's an infinite loop of the drawer just opening and closing.
It's not really worth fixing, but just kinda funny.
Also, yeah I've thought about this implementation more, I think the idea of using invisible navbars to measure width should be fine I think
In regards to this comment, the solution I suggested to use So the goal here is to only apply the The way I would suggest is go through the components of className={cn(
// ... all the previous classes
orientation === 'horizontal' && "hover:text-helpmeblue", // was previously "md:hover:text-helpmeblue"
className,
)}Just make sure that these classes are added near the end of the The reason why this works:
|
|
Thank you for the comments and suggestions! I have implemented the requested navbar review changes:
I manually tested desktop/compact/drawer transitions, drawer layout, and queue dropdown behavior and did not find issues. |
| 'inset-x-0 bottom-0 mt-24 rounded-t-[10px]', | ||
| direction === 'left' && | ||
| 'left-0 top-0 h-full w-[60vw] max-w-80 rounded-r-[10px]', | ||
| 'left-0 top-0 max-h-[100dvh] w-[60vw] max-w-80 overflow-y-auto overflow-x-hidden rounded-r-[10px]', |
There was a problem hiding this comment.
Why is the sidebar now... short? I mentioned there should be a shorter max width, not height. probably just changing it from max-w-80 to max-w-72 or something.
There was a problem hiding this comment.
Oh sorry, I misread it as reducing the height and getting rid of the blank white space between the elements and the Profile element. I spent a lot of time on this, my bad. I'll fix it right away.
There was a problem hiding this comment.
I have made the height full window and reduced the width (w-[46vw] max-w-60).
AdamFipke
left a comment
AdamFipke
left a comment
There was a problem hiding this comment.
just one small comment, should be good to merge after
| const queueDropdownListClass = showDrawerPresentation | ||
| ? `grid w-full gap-1 p-4 ${sortedQueues.length > 6 ? 'grid-cols-2' : 'grid-cols-1'}` | ||
| : `grid gap-1 p-4 md:grid-cols-2 lg:w-[600px] lg:gap-2 ${sortedQueues.length > 6 ? 'w-[95vw] grid-cols-2' : 'w-[60vw]'}` | ||
| const emptyQueueStateClass = showDrawerPresentation | ||
| ? 'w-full p-4 text-center text-sm text-gray-500' | ||
| : `w-[60vw] p-4 text-center text-sm text-gray-500 ${role === Role.PROFESSOR ? 'lg:w-[600px]' : 'lg:w-[400px]'}` |
There was a problem hiding this comment.
wait i'm confused. Why are these here if they're only used in one area? I feel like it'd be a lot more maintainable to keep the classname on the elements themselves generally. You can also use the cn helper function for formatting conditional classnames nicely. Same goes for horizontalInsetImportantClass.
The rest are probably fine to leave as they are since they're used in multiple areas but in general you could go either way
There was a problem hiding this comment.
Thank you for pointing this out, used cn(...) to inline the single-use classnames in HeaderBar.tsx and kept the reused shared class constants extracted.
| className={ | ||
| isACourseSettingsPage | ||
| ? // the hover:border-none is because the inner link has a hover effect that adds another border | ||
| 'md:border-helpmeblue bg-zinc-300/80 md:border-b-2 md:bg-white md:hover:border-none md:focus:border-none' |
There was a problem hiding this comment.
I tried ctrl+fing "border-helpmeblue" and I see that the original desktop styles have actually been completely removed (presumably got misinterpreted for some reason after my first round of feedback). I was expecting that the changes were at least tested. I'll look into fixing it, but I might just revert the changes done by this PR
There was a problem hiding this comment.
Yeah, the more I look at it now, the more random broken changes I see. It's not worth my time to try to salvage this.
2 participants
Description
This PR fixes the responsive course header behavior for issue #418. Previously, the header switched to the drawer menu only at a fixed breakpoint (768px). On tablet/smaller desktop widths, role/course-dependent nav items (especially professor links like Course Settings and Insights) could overflow, causing a horizontal scrollbar and dropdown placement issues.
The header now uses a 3-mode system:
Mode selection is based on measured rendered width (not fixed nav-item-count estimates), so it helps with student vs professor nav differences, enabled course features, zoom/larger font size scenarios.
Additionally, NavigationMenu now supports an optional showViewport prop so hidden measurement navbars can skip rendering an extra viewport element.
No dependencies, env changes, DB changes, or install steps are required.
Closes #418
Screenshots
All screenshots show no horizontal scrollbar and correct nav mode for width/role.
Type of change
yarn installHow Has This Been Tested?
Manually tested on course pages with different roles and feature sets, focusing on widths where overflow previously occurred.
Repro steps:
Zoom testing note: I could only test zoom reliably at desktop width. At 100% zoom, all nav elements are shown in desktop mode; after zooming in enough, the navbar shifts to drawer mode without introducing a horizontal scrollbar. I couldn’t reliably test zoom at other widths in DevTools device mode.
Checklist:
console.logs, leftover unused logic, or anything else that was accidentally committed)