Implement navigation changes #267
Conversation
… parts of the app This is a challenge because the different navbars need different data as part of the route loader (ie, `useLoaderData` return value), so there's not a good way to abstract this without tying into the router somewhere.
… new srcbook url Previously, cells and channel data were still being stored from the initial srcbook, not the one that was newly navigated towards.
…wice when clicking "npm inatall"
… in sheet mode This is what it looked like in the designs!
1egoman
force-pushed
the
navigation-changes
branch
from
703742e to
217796a
Compare
| errorElement: <ErrorPage />, | ||
| }, | ||
| { | ||
| path: '/secrets', | ||
| loader: Secrets.loader, | ||
| element: <Secrets />, | ||
| errorElement: <ErrorPage />, | ||
| }, | ||
| { | ||
| path: '/settings', | ||
| element: <Settings />, | ||
| loader: Settings.loader, | ||
| action: Settings.action, | ||
| errorElement: <ErrorPage />, | ||
| path: '/', | ||
| element: ( | ||
| <LayoutNavbar> | ||
| <Outlet /> | ||
| </LayoutNavbar> | ||
| ), | ||
| loader: configLoader, | ||
| children: [ | ||
| { | ||
| path: '/secrets', | ||
| loader: Secrets.loader, | ||
| element: <Secrets />, | ||
| errorElement: <ErrorPage />, | ||
| }, | ||
| { | ||
| path: '/settings', | ||
| element: <Settings />, | ||
| loader: Settings.loader, | ||
| action: Settings.action, | ||
| errorElement: <ErrorPage />, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
I've had to make some relatively substantial changes to the root router at the top of the app - this is because I split the navbar up into two different underlying implementations, one that renders on the session page, and one that renders everywhere else, and the two different implementations require different data (which meant I had to use two different loaders depending on the navbar).
There may be a more elegant way to do this, but what I have here seems to work.
| const { config, srcbooks, session } = useLoaderData() as SessionLoaderDataType; | ||
|
|
||
| // Because we use refs for our state, we need a way to trigger | ||
| // component re-renders when the ref state changes. | ||
| // | ||
| // https://legacy.reactjs.org/docs/hooks-faq.html#is-there-something-like-forceupdate | ||
| // | ||
| const [, forceComponentRerender] = useReducer((x) => x + 1, 0); | ||
|
|
||
| const channelRef = useRef(SessionChannel.create(session.id)); | ||
| const connectedSessionIdRef = useRef<SessionType['id'] | null>(null); | ||
| const connectedSessionLanguageRef = useRef<SessionType['language'] | null>(null); | ||
| const channel = channelRef.current; | ||
|
|
||
| useEffectOnce(() => { | ||
| useEffect(() => { | ||
| if (connectedSessionIdRef.current === session.id) { | ||
| return; | ||
| } | ||
|
|
||
| const oldChannel = channelRef.current; | ||
|
|
||
| // Disconnect from the previously connected session | ||
| if (connectedSessionIdRef.current) { | ||
| oldChannel.unsubscribe(); | ||
|
|
||
| if (connectedSessionLanguageRef.current === 'typescript') { | ||
| oldChannel.push('tsserver:stop', { sessionId: session.id }); | ||
| } | ||
| } | ||
|
|
||
| // Reconnect to the new session | ||
| channelRef.current = SessionChannel.create(session.id); | ||
| connectedSessionIdRef.current = session.id; | ||
| connectedSessionLanguageRef.current = session.language; | ||
|
|
||
| const channel = channelRef.current; | ||
|
|
||
| channel.subscribe(); | ||
|
|
||
| if (session.language === 'typescript') { | ||
| channel.push('tsserver:start', { sessionId: session.id }); | ||
| } | ||
|
|
||
| return () => { | ||
| channel.unsubscribe(); | ||
|
|
||
| if (session.language === 'typescript') { | ||
| channel.push('tsserver:stop', { sessionId: session.id }); | ||
| } | ||
| }; | ||
| }); | ||
| forceComponentRerender(); | ||
| }, [session.id, session.language, forceComponentRerender]); |
There was a problem hiding this comment.
I had to make some signifigant changes to the way that the session page connected to the underlying websocket - the way it previously worked, if a user navigated internally within the app to a new srcbook (ie, by clicking a link, etc), the socket wouldn't disconnect and reconnect, so messages related to a new srcbook would be going to a misconfigured socket, leading to the dev server crashing.
This I think needs a fair amount of testing! As far as I can tell it works locally for me, but I am not sure if I am exercising all the cases.
| export const SESSION_MENU_PANELS = [ | ||
| { | ||
| name: 'tableOfContents' as const, | ||
| icon: ListIcon, | ||
| openWidthInPx: 312, | ||
| contents: () => <SessionMenuPanelTableOfContents />, | ||
| }, | ||
| { | ||
| name: 'packages' as const, | ||
| icon: PackageIcon, | ||
| openWidthInPx: 480, | ||
| contents: ({ session, openDepsInstallModal }: SessionMenuPanelContentsProps) => ( | ||
| <SessionMenuPanelPackages session={session} openDepsInstallModal={openDepsInstallModal} /> | ||
| ), | ||
| }, | ||
| { | ||
| name: 'settings' as const, | ||
| icon: SettingsIcon, | ||
| openWidthInPx: 480, | ||
| contents: (props: SessionMenuPanelContentsProps) => <SessionMenuPanelSettings {...props} />, | ||
| }, | ||
| // NOTE: re-enable this in the follow up change! | ||
| // { | ||
| // name: 'secrets' as const, | ||
| // icon: KeySquareIcon, | ||
| // openWidthInPx: 480, | ||
| // contents: () => <SessionMenuPanelSecrets />, | ||
| // }, | ||
| ]; |
There was a problem hiding this comment.
This data structure controls rendering all the side panels. For now, the secrets panel is disabled.
| {props.srcbooks | ||
| .sort((a, b) => b.openedAt - a.openedAt) | ||
| .slice(0, 6) | ||
| .map((srcbook) => { | ||
| if (srcbook.id === props.session.id) { | ||
| return null; | ||
| } | ||
| const titleCell = srcbook.cells.find((cell) => cell.type === 'title') as | ||
| | TitleCellType | ||
| | undefined; | ||
| if (!titleCell) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <DropdownMenuItem | ||
| key={srcbook.id} | ||
| onClick={() => navigate(`/srcbooks/${srcbook.id}`)} | ||
| > | ||
| {titleCell.text} | ||
| </DropdownMenuItem> | ||
| ); | ||
| })} | ||
|
|
||
| {/* FIXME: how should more than 6 entries be rendered? */} | ||
| {props.srcbooks.length > 6 ? ( | ||
| <DropdownMenuItem asChild> | ||
| <Link to="/">More...</Link> | ||
| </DropdownMenuItem> | ||
| ) : null} |
There was a problem hiding this comment.
Right now, only the 6 most recent entries are rendered in the inline srcbook picker dropdown. It's unclear from the designs if this is the intention or not, but given this probably should be length limited in some capacity to keep it from overflowing the screen, I limited it to 6, and if there's more than 6, rendered a More... item that just goes back to the home view.
1egoman
force-pushed
the
navigation-changes
branch
from
3587e44 to
835e7f4
Compare
| {/* At the xl breakpoint, the sessionMenu appears inline so we pad left to balance*/} | ||
| <div className="grow shrink lg:px-0 pb-28"> | ||
| <div className="max-w-[800px] mx-auto my-12 px-[32px]"> | ||
| <TitleCell cell={titleCell} updateCellOnServer={updateCellOnServer} /> |
There was a problem hiding this comment.
This may have been there before (though I never noticed it if so) but the title cell appears to break at any character.
Nothing here stands out as having done introduced this but possibly related to the styles applied above?
There was a problem hiding this comment.
It looks like this behavior currently happens in main:
That being said, I can take a look if desired - word-wrap: break-word might do what you are looking for here?
There was a problem hiding this comment.
Ok we can do that in separate PR then.
This change implements a new global navbar and sidebar, based on the designs in figma.
This is what the main session page (
/srcbooks/:id) looks like now:When a user clicks on any of the sidebar icons and the screen is thinner, a sheet opens:
When a user clicks on any of the sidebar icons and the screen is wider, a persistently open panel is shown:
Finally, the navbar carries over to the home page in addition to the session page:
