Skip to content

Make sidebars responsive #17010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 24, 2023
Merged

Make sidebars responsive #17010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7194c10
make sidebar responsive
selfcontained Mar 23, 2023
204ca75
adding separator after submenu on small screens
selfcontained Mar 23, 2023
05387af
make heading smaller on small screens
selfcontained Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 52 additions & 15 deletions components/dashboard/src/components/PageWithSubMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
* See License.AGPL.txt in the project root for license information.
*/

import classNames from "classnames";
import { FC, useEffect, useRef } from "react";
import { useLocation } from "react-router";
import { Link } from "react-router-dom";
import Header, { TabEntry } from "../components/Header";
import { Separator } from "./Separator";

export interface PageWithSubMenuProps {
title: string;
Expand All @@ -20,30 +23,64 @@ export interface PageWithSubMenuProps {
}

export function PageWithSubMenu(p: PageWithSubMenuProps) {
const location = useLocation();
return (
<div className="w-full">
<Header title={p.title} subtitle={p.subtitle} tabs={p.tabs} />
<div className="app-container flex pt-9">
<div className="app-container flex md:pt-9 flex-col md:flex-row">
{/* TODO: extract into SubMenu component and show scrolling indicators */}
<div>
<ul className="flex flex-col text tracking-wide text-gray-500 pt-4 lg:pt-0 w-52 space-y-2">
<ul
className={classNames(
// Handle flipping between row and column layout
"flex flex-row md:flex-col items-center",
"w-full md:w-52 overflow-auto md:overflow-visible",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What do you think of making the horizontal scrollbar invisible? This works ok now with the pill-like design, but could clash with the tabs.

Screenshot 2023-03-24 at 13 06 10

"pt-4 pb-4 md:pb-0",
"space-x-2 md:space-x-0 md:space-y-2",
"tracking-wide text-gray-500",
)}
>
{p.subMenu.map((e) => {
let classes = "flex block py-2 px-4 rounded-md";
if (e.link.some((l) => l === location.pathname)) {
classes += " bg-gray-300 text-gray-800 dark:bg-gray-800 dark:text-gray-50";
} else {
classes += " hover:bg-gray-100 dark:hover:bg-gray-800";
}
return (
<Link to={e.link[0]} key={e.title}>
<li className={classes}>{e.title}</li>
</Link>
);
return <SubmenuItem title={e.title} link={e.link} key={e.title} />;
})}
</ul>
</div>
<div className="ml-16 lg:ml-32 w-full pt-1 mb-40">{p.children}</div>
<div className="md:ml-16 lg:ml-32 w-full pt-1 mb-40">
<Separator className="md:hidden" />
<div className="pt-4 md:pt-0">{p.children}</div>
</div>
</div>
</div>
);
}

type SubmenuItemProps = {
title: string;
link: string[];
};

const SubmenuItem: FC<SubmenuItemProps> = ({ title, link }) => {
const location = useLocation();
const itemRef = useRef<HTMLLIElement>(null);

// TODO: can remove this once we use sub-routes and don't re-render the whole page for each sub-route
useEffect(() => {
if (itemRef.current && link.some((l) => l === location.pathname)) {
itemRef.current.scrollIntoView({ behavior: "auto", block: "nearest", inline: "start" });
}
}, [link, location.pathname]);

let classes = "flex block py-2 px-4 rounded-md whitespace-nowrap max-w-52";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Probably not the best line to comment on this, but the initial idea waw to reuse the same tabs pattern we use in the dashboard. It should be ok to leave is as is for this PR but it'd be nicer for users to face a pattern they've seen before. Thoughts?

BEFORE AFTER
bmh-respon4a5450a2ee preview gitpod-dev com_usage(Pixel 5) bmh-respon4a5450a2ee preview gitpod-dev com_usage(Pixel 5) (1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, that def. looks better. What do you think you'd prefer here for the sidebar nav between the tab-bar or a dropdown?

  • A dropdown avoids some of the weird scroll positioning issues when you navigate described in another comment (which we can fix, but might take a bit).
  • The tab-bar nav options are a bit more discoverable since you can see (some of) them.

🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Going with the tabs pattern will also help decrease the gap between the menu items and providing a hint for menu items overflowing the viewport.


if (link.some((l) => l === location.pathname)) {
classes += " bg-gray-300 text-gray-800 dark:bg-gray-800 dark:text-gray-50";
} else {
classes += " hover:bg-gray-100 dark:hover:bg-gray-800";
}
return (
<Link to={link[0]} key={title} className="md:w-full">
<li ref={itemRef} className={classes}>
{title}
</li>
</Link>
Comment on lines +80 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Probably not the best line to comment about this, but could we not shift the menu items when an item is clicked? Noticed that clicking on an item moves it to the left side of the scrollable area, which sometimes seems a bit jarring, no?

Untitled.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, I didn't like this either, tried to explain it a bit in the PR description, but it's buried in there:

Currently navigation on the submenu results in a full route re-render, which resets the scroll position of the menu. This PR shifts the active item into visibility, but it isn't able to maintain scroll position due to the re-render. We can address this in a followup by using nested routes w/ react-router, and only re-render the portions of the page that change as much as possible.

Another thing to consider is if we did a dropdown for nav instead of this scrollbar or a tab-bar, it would avoid that issue.

);
};
2 changes: 1 addition & 1 deletion components/dashboard/src/components/typography/headings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export const Heading1: FC<HeadingProps> = ({ color, tracking, className, childre
className={classNames(
getHeadingColor(color),
getTracking(tracking),
"font-bold text-5xl leading-64",
"font-bold text-4xl md:text-5xl leading-64",
className,
)}
>
Expand Down