-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
URL bar improvements #526
URL bar improvements #526
Changes from all commits
a66e01e
9c787f4
ce2dc18
967644e
def9ac1
3e9f547
20113ce
01aaa91
0388d0b
4ac59a3
d47c878
3086be6
d6b7fe0
22c35f1
0e70dbd
809cdce
739dcc3
544e920
5dd5272
7efd05b
c003b87
e0e5a6d
476ea1b
68ad7ca
57ff8d3
fa0a164
663d217
d144cb2
33f98bc
6c51a81
6972b78
4ef0333
7640e6b
7e89e30
3f7447c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,11 @@ | ||
import { useEffect, useState } from "react"; | ||
import IconButton from "./shared/IconButton"; | ||
import { ProjectInterface, ProjectState } from "../../common/Project"; | ||
import UrlSelect from "./UrlSelect"; | ||
import * as DropdownMenu from "@radix-ui/react-dropdown-menu"; | ||
import { useEffect, useState, useMemo } from "react"; | ||
import { useProject } from "../providers/ProjectProvider"; | ||
import UrlSelect, { UrlItem } from "./UrlSelect"; | ||
import { IconButtonWithOptions } from "./IconButtonWithOptions"; | ||
import IconButton from "./shared/IconButton"; | ||
|
||
interface UrlBarProps { | ||
project: ProjectInterface; | ||
disabled?: boolean; | ||
} | ||
|
||
interface ReloadButtonProps { | ||
project: ProjectInterface; | ||
disabled: boolean; | ||
} | ||
|
||
function ReloadButton({ project, disabled }: ReloadButtonProps) { | ||
function ReloadButton({ disabled }: { disabled: boolean }) { | ||
const { project } = useProject(); | ||
return ( | ||
<IconButtonWithOptions | ||
onClick={() => project.restart(false)} | ||
|
@@ -35,28 +25,57 @@ function ReloadButton({ project, disabled }: ReloadButtonProps) { | |
); | ||
} | ||
|
||
function UrlBar({ project, disabled }: UrlBarProps) { | ||
const [urlList, setUrlList] = useState<{ name: string; id: string }[]>([]); | ||
function UrlBar({ disabled }: { disabled?: boolean }) { | ||
const { project } = useProject(); | ||
|
||
const MAX_URL_HISTORY_SIZE = 20; | ||
const MAX_RECENT_URL_SIZE = 5; | ||
|
||
const [backNavigationPath, setBackNavigationPath] = useState<string>(""); | ||
const [urlList, setUrlList] = useState<UrlItem[]>([]); | ||
const [recentUrlList, setRecentUrlList] = useState<UrlItem[]>([]); | ||
const [urlHistory, setUrlHistory] = useState<string[]>([]); | ||
|
||
useEffect(() => { | ||
function moveAsMostRecent(urls: UrlItem[], newUrl: UrlItem) { | ||
return [newUrl, ...urls.filter((record) => record.id !== newUrl.id)]; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it would be better to write it like this:
I made a simple benchmark to check the approaches:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't agree, trading 2ms of performance for worse readability isn't worth it. Especially when you tested it on 1k long list, we won't get anywhere that list length ever. |
||
|
||
function handleNavigationChanged(navigationData: { displayName: string; id: string }) { | ||
const newRecord = { name: navigationData.displayName, id: navigationData.id }; | ||
setUrlList((urlList) => [ | ||
newRecord, | ||
...urlList.filter((record) => record.id !== newRecord.id), | ||
]); | ||
if (backNavigationPath && backNavigationPath !== navigationData.id) { | ||
return; | ||
} | ||
|
||
const newRecord: UrlItem = { | ||
name: navigationData.displayName, | ||
id: navigationData.id, | ||
}; | ||
const isNotInHistory = urlHistory.length === 0 || urlHistory[0] !== newRecord.id; | ||
|
||
setUrlList((currentUrlList) => moveAsMostRecent(currentUrlList, newRecord)); | ||
setRecentUrlList((currentRecentUrlList) => { | ||
const updatedRecentUrls = moveAsMostRecent(currentRecentUrlList, newRecord); | ||
return updatedRecentUrls.slice(0, MAX_RECENT_URL_SIZE); | ||
}); | ||
|
||
if (isNotInHistory) { | ||
setUrlHistory((currentUrlHistoryList) => { | ||
const updatedUrlHistory = [newRecord.id, ...currentUrlHistoryList]; | ||
return updatedUrlHistory.slice(0, MAX_URL_HISTORY_SIZE); | ||
}); | ||
} | ||
setBackNavigationPath(""); | ||
} | ||
|
||
project.addListener("navigationChanged", handleNavigationChanged); | ||
const handleProjectReset = (e: ProjectState) => { | ||
if (e.status === "starting") { | ||
setUrlList([]); | ||
} | ||
}; | ||
project.addListener("projectStateChanged", handleProjectReset); | ||
return () => { | ||
project.removeListener("navigationChanged", handleNavigationChanged); | ||
project.removeListener("projectStateChanged", handleProjectReset); | ||
}; | ||
}, []); | ||
}, [recentUrlList, urlHistory, backNavigationPath]); | ||
|
||
const sortedUrlList = useMemo(() => { | ||
return [...urlList].sort((a, b) => a.name.localeCompare(b.name)); | ||
}, [urlList]); | ||
|
||
return ( | ||
<> | ||
|
@@ -65,34 +84,37 @@ function UrlBar({ project, disabled }: UrlBarProps) { | |
label: "Go back", | ||
side: "bottom", | ||
}} | ||
disabled={disabled || urlList.length < 2} | ||
disabled={disabled || urlHistory.length < 2} | ||
onClick={() => { | ||
project.openNavigation(urlList[1].id); | ||
// remove first item from the url list | ||
setUrlList((urlList) => urlList.slice(1)); | ||
setUrlHistory((prevUrlHistory) => { | ||
const newUrlHistory = prevUrlHistory.slice(1); | ||
setBackNavigationPath(newUrlHistory[0]); | ||
project.openNavigation(newUrlHistory[0]); | ||
return newUrlHistory; | ||
}); | ||
}}> | ||
<span className="codicon codicon-arrow-left" /> | ||
</IconButton> | ||
<ReloadButton project={project} disabled={disabled ?? false} /> | ||
<ReloadButton disabled={disabled ?? false} /> | ||
<IconButton | ||
onClick={() => { | ||
project.goHome(); | ||
setUrlList([]); | ||
project.goHome("/{}"); | ||
}} | ||
tooltip={{ | ||
label: "Go to main screen", | ||
side: "bottom", | ||
}} | ||
disabled={disabled || urlList.length == 0}> | ||
disabled={disabled || urlList.length < 2}> | ||
<span className="codicon codicon-home" /> | ||
</IconButton> | ||
<UrlSelect | ||
onValueChange={(value: string) => { | ||
project.openNavigation(value); | ||
}} | ||
items={urlList} | ||
recentItems={recentUrlList} | ||
items={sortedUrlList} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're filtering out URLs if they have no name, do we want to store them then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When changing from a preview to another URL, two route changes happen: {displayName: '', id: '{}'} and {displayName: '/', id: '/{}'}. We skip storing the first route with an empty displayName to prevent saving home URL entries twice. |
||
value={urlList[0]?.id} | ||
disabled={disabled || urlList.length < 1} | ||
disabled={disabled || urlList.length < 2} | ||
/> | ||
</> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function alone does not give as information if project uses expo router or not