-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add Open Deep Link modal #640
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
While I haven't reviewed the code yet, wanted to add some UX related comments based on the videos you attached:
- It doesn't feel good when the height of the dialog changes when you type (there are less choices as you narrow down the search, but shifting the modal makes it difficult to type or follow).
- I think it'd make sense for this to work similarily to the browser URL bar, specifically it doesn't seem like we really need to input fields for the URL and for search separately. The main one should be a typeahead (using past history) input
… shared component
I pushed some changes, the modal now has static height and there is only one input field which acts kind of like a search bar in browser (I tried to match the behavior of https://github.com/junegunn/fzf). |
From the UX perspective it makes little sense to have the magnifier glass icon there. It is an input field that also narrows down the history list. I'd perhaps just leave it without any icon and instead add placeholder "Enter deeplink or search history" I'd also experiment with adding some label for the list of previous entries such that it is clear what we show there. For example "History" or "Previously used URLs" |
@kmagiera what do you think about something like this? |
Looks good! |
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.
Looks good. Left some tiny inline comments to address before merging
); | ||
} | ||
|
||
this.deviceSession?.sendDeepLink(link); |
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.
do we want to await here for consistency?
I will get back to this once #675 is merged so we can use app level cache for storing recently used deep links |
There's no reason to wait, this is accepted for a few days already. #675 only changes the storage for build result metadata, we're still storing other settings and data in workspace-specific storage. It doesn't make sense to put URLs storage in that cache leaving other settings in workspace cache as it is conceptually closer to device settings than it is to native builds. |
Ok then I'm merging this one. When the app level cache is created and we feel a need to move app level settings there from workspace state, then we can move cached deep links too. |
Resolves #631
Changes
<SearchSelect/>
, which is used in Open Deep Link modal.abc
this is matching to regex*a*b*c*
).Demos (dark and light mode)
deeplinks_demo_dark.mov
deeplinks_demo_light.mov
How Has This Been Tested:
Tested in light and dark mode, on Iphone 15 Pro sim and Pixel 9 emulator, on projects
expo-router
andreact-native-75
.