Fix UrlSelect trigger value reset for non expo router projects #873
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| value: string; | ||
| onValueChange: (newValue: string) => void; | ||
| recentItems: UrlItem[]; | ||
| items: UrlItem[]; | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| function UrlSelect({ onValueChange, recentItems, items, value, disabled }: UrlSelectProps) { | ||
| function UrlSelect({ onValueChange, recentItems, items, key, value, disabled }: UrlSelectProps) { |
There was a problem hiding this comment.
I believe that it only has been added in React 19 that you can receive key prop like this. I don't think in the current version in propagates down to the component
| @@ -35,7 +36,7 @@ function UrlSelect({ onValueChange, recentItems, items, value, disabled }: UrlSe | |||
| }; | |||
|
|
|||
| return ( | |||
| <Select.Root onValueChange={handleValueChange} value={value} disabled={disabled}> | |||
| <Select.Root onValueChange={handleValueChange} key={key} value={value} disabled={disabled}> | |||
There was a problem hiding this comment.
It should be enough to do something along these lines, instead of having the whole logic for urlSelectKey in the parent component:
<Select.root key={value ? "has-value : "no-value"}>| @@ -117,8 +130,9 @@ function UrlBar({ disabled }: { disabled?: boolean }) { | |||
| }} | |||
| recentItems={recentUrlList} | |||
| items={sortedUrlList} | |||
| value={urlList[0]?.id} | |||
| disabled={disabledAlsoWhenStarting || urlList.length < 2} | |||
| key={urlSelectKey} | |||
There was a problem hiding this comment.
setting this here is enough to re-mount the whole component. It doesn't have to be accessed in the underlying component
|
I’ve removed the urlSelectKey state and key prop from UrlSelect, which were added in a previous PR version. Setting setUrlSelectValue to an empty string is sufficient to reset the trigger value of UrlSelect to a placeholder. |
There was a problem hiding this comment.
I approve this changes adding some inline comments to adres #887.
| @@ -103,6 +109,9 @@ function UrlBar({ disabled }: { disabled?: boolean }) { | |||
| <IconButton | |||
| onClick={() => { | |||
| project.goHome("/{}"); | |||
| if (!isExpoRouterProject) { | |||
There was a problem hiding this comment.
I don't like this being placed as a dependency of a user action as we'll have to remember to update it every time we add a new navigation option I would much rather have wrapper.js send an event every time application is loaded and then some global logic would handle setting the correct navigation state to the url bar.
Having said that I think we should move most of the logic in this file to the extension code to allow passing state between diffrent panel locations any wayand my suggestion will be easier to active then.
p-malecki
deleted the
@p-malecki/fix-navigation-for-non-expo-router-projects
branch
This PR changes the UrlBar behaviour in projects not using Expo Router navigation.
When the user intends to close the preview by pressing the goHome button (or reload), the UrlSelect's trigger value needs to be reset to a placeholder ("/").
Additionally, this update disables the
goBackbutton for projects that do not use Expo Router.How Has This Been Tested:
Screen.Recording.2024-12-20.at.18.45.00.mov
Screen.Recording.2024-12-20.at.18.35.50.mov