Skip to content
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

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Add Open Deep Link modal #640

merged 10 commits into from
Nov 4, 2024

Conversation

km1chno
Copy link
Contributor

@km1chno km1chno commented Oct 21, 2024

Resolves #631

Changes

  • Adds shared component <SearchSelect/>, which is used in Open Deep Link modal.
  • The component allows to select option from scrollable list and provides search bar, which performs some simple version of fuzzy search, it doesn't support matching with misspellings, but allows for holes (basically for search phrase abc this is matching to regex *a*b*c*).
  • Adds "Open Deep Link" modal that allows to input a deep link to open in app and browse history of recently opened deep links, which is cached in workspace state (only last 50 entries are cached).

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 and react-native-75.

Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 0:35am

@km1chno km1chno self-assigned this Oct 21, 2024
Copy link
Member

@kmagiera kmagiera left a 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:

  1. 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).
  2. 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

@km1chno
Copy link
Contributor Author

km1chno commented Oct 28, 2024

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).

@kmagiera
Copy link
Member

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"

@km1chno
Copy link
Contributor Author

km1chno commented Oct 30, 2024

@kmagiera what do you think about something like this?

Zrzut ekranu 2024-10-30 o 11 36 32

@kmagiera
Copy link
Member

Looks good!

Copy link
Member

@kmagiera kmagiera left a 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);
Copy link
Member

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?

@km1chno
Copy link
Contributor Author

km1chno commented Nov 4, 2024

I will get back to this once #675 is merged so we can use app level cache for storing recently used deep links

@kmagiera
Copy link
Member

kmagiera commented Nov 4, 2024

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.

@km1chno
Copy link
Contributor Author

km1chno commented Nov 4, 2024

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.

@km1chno km1chno merged commit 38d7e35 into main Nov 4, 2024
4 checks passed
@km1chno km1chno deleted the @km1chno/add-open-deep-link-modal branch November 4, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Deep Link dialog
2 participants