-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat(wallet-mobile): Add dApp Explorer feature flag and rename variables #3190
Conversation
const {isEdit} = router.params ?? {isEdit: false} | ||
const navigateTo = useNavigateTo() | ||
const {addBrowserTab, setTabActive, updateTab, tabs, tabActiveIndex} = useBrowser() | ||
const tabActive = tabs[tabActiveIndex] | ||
const [searchValue, setSearchValue] = useState('') | ||
const [searchValue, setSearchValue] = React.useState('') |
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.
heads up: we have a hook + provider that transforms the navbar into a searchable bar. (dunno if it's applied on nav bar this one)
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 have the hook used in SelectDappFromListScreen
, but this component is the search in the browser, which has a different UI, so I'd say it's okay to use a custom component
apps/wallet-mobile/src/features/Discover/useCases/BrowseDapp/BrowserToolbar.tsx
Show resolved
Hide resolved
apps/wallet-mobile/src/features/Discover/useCases/BrowseDapp/BrowserToolbar.tsx
Show resolved
Hide resolved
@@ -23,7 +23,7 @@ export const BrowserTabsBar = () => { | |||
|
|||
const handleCreateTab = () => { | |||
switchTab(false) | |||
navigateTo.browserSearch(false) | |||
navigateTo.searchDappInBrowser({isEdit: false}) |
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.
params are unsafe, later if possible move to state
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'll refactor this. It looks like we don't need params nor state
screen: 'browser-view', | ||
}), | ||
selectDappFromList: () => navigation.navigate('discover-select-dapp-from-list'), | ||
browseDapp: () => navigation.navigate('discover-browser', {screen: 'discover-browse-dapp'}), | ||
navigation: () => navigation, |
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 should be deleted.
screen: 'browser-view', | ||
}), | ||
selectDappFromList: () => navigation.navigate('discover-select-dapp-from-list'), | ||
browseDapp: () => navigation.navigate('discover-browser', {screen: 'discover-browse-dapp'}), | ||
navigation: () => navigation, |
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.
navigation: () => navigation, |
@@ -106,7 +106,7 @@ export const BrowserProvider = ({ | |||
} | |||
|
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.
@michaeljscript this is looking confusing and with a lot of var shadowing, can you give it a look?
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.
Updated
Resolves YOMO-1296