-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
Warning: on small window sizes ellipsis may be not visible with long url labels.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -237,7 +237,7 @@ export class Project | |||
} | |||
|
|||
public async goHome() { | |||
await this.reloadMetro(); | |||
await this.openNavigation("/{}"); |
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.
Are we sure that it always doesn't take any initial args?
<span className="codicon codicon-home" /> | ||
</IconButton> | ||
<UrlSelect | ||
onValueChange={(value: string) => { | ||
project.openNavigation(value); | ||
}} | ||
items={urlList} | ||
recentItems={recentUrlList} |
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.
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 comment
The 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.
if (this.expoRouterInstalled) { | ||
await this.openNavigation(homeUrl); | ||
} |
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.
What if it's not installed?
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.
It that case we reload metro. I've changed it.
Co-authored-by: Jakub Gonet <[email protected]>
export function isExpoRouterProject() { | ||
try { | ||
const appRoot = getAppRootFolder(); | ||
const packageJson = require(path.join(appRoot, "package.json")); |
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.
requireNoCache
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.
left some inline comments please address them before marging
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it would be better to write it like this:
const result = urls.filter((record) => record.id !== newUrl.id);
result.unshift(newUrl);
return result
I made a simple benchmark to check the approaches:
let testCases = new Array(1000).fill(0).map(()=>{
const list = new Array(1000).fill(0);
return list.map((e,i)=>{
return i;
});
});
const movedURL = 560;
let start = performance.now();
for ( let testCase of testCases){
const newTestCase = [ movedURL, ...testCase.filter((element)=>element !==movedURL)];
}
let end = performance.now();
testCases = new Array(1000).fill(0).map(()=>{
const list = new Array(1000).fill(0);
return list.map((e,i)=>{
return i;
});
});
console.log("old approach",start - end);
start = performance.now();
for ( let testCase of testCases){
const filtered = testCase.filter((element)=>element !==movedURL);
filtered.unshift(movedURL);
const newTestCase = filtered;
}
end = performance.now();
console.log("new approach",start - end);
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.
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.
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.
I aprove but left some coments
const appRoot = getAppRootFolder(); | ||
const packageJson = requireNoCache(path.join(appRoot, "package.json")); | ||
const hasExpoRouter = Object.values<string>(packageJson.dependencies).some( | ||
(dependency) => dependency === "expo-router" |
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.
can expo router ever be a dev dependency?
try { | ||
const appRoot = getAppRootFolder(); | ||
const packageJson = requireNoCache(path.join(appRoot, "package.json")); | ||
const hasExpoRouter = Object.values<string>(packageJson.dependencies).some( |
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.
values are versions you wat to use keys. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys
This PR introduces several enhancements to the URL bar:
home
button in expo router projects opens the root URL (/{}
) with empty params but triggers a metro reload in projects without expo router.Initial params may be may be introduced in future PRs.
go back
button navigates back through the URL history up to alimit of 20 URLs.
Select.Content
in the URL bar, followed by an alphabetically sortedlist of all visited URLs.
Select.Content
andSelect.Trigger
components.
Select.Content
.Select.Content
.Expo Router
as an optional dependency to the diagnostics view.Screenshots:
Test Plan:
home button
redirects to the root URL in project using Expo Router and the other without.go back
button to validate correct navigation through the URL history (additionally, test with dynamic links and component previews).