feat: Safe Apps landing page #3764
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
DaniSomoza
requested review from
katspaugh,
iamacook,
DiogoSoaress,
dasanra and
usame-algan
src/routes/routes.ts
Outdated
| @@ -44,6 +44,7 @@ export const ROOT_ROUTE = '/' | |||
| export const WELCOME_ROUTE = '/welcome' | |||
| export const OPEN_SAFE_ROUTE = '/open' | |||
| export const LOAD_SAFE_ROUTE = generatePath(LOAD_SPECIFIC_SAFE_ROUTE) // By providing no slug, we get '/load' | |||
| export const SAFE_APP_LANDPAGE_ROUTE = '/share/safe-app' | |||
There was a problem hiding this comment.
I would rename this to SAFE_APP_LANDING_PAGE_ROUTE.
| const { pathname } = useLocation() | ||
|
|
||
| const showWalletSection = !matchPath(pathname, { | ||
| path: [SAFE_APP_LANDPAGE_ROUTE], | ||
| }) |
There was a problem hiding this comment.
| const { pathname } = useLocation() | |
| const showWalletSection = !matchPath(pathname, { | |
| path: [SAFE_APP_LANDPAGE_ROUTE], | |
| }) | |
| const showWalletSelection = !useRouteMatch({ path: SAFE_APP_LANDPAGE_ROUTE }) |
src/components/AppLayout/index.tsx
Outdated
| const showSideBar = !matchPath(pathname, { | ||
| path: [SAFE_APP_LANDPAGE_ROUTE], | ||
| }) |
There was a problem hiding this comment.
| const showSideBar = !matchPath(pathname, { | |
| path: [SAFE_APP_LANDPAGE_ROUTE], | |
| }) | |
| const showWalletSelection = !useRouteMatch({ path: SAFE_APP_LANDPAGE_ROUTE }) |
| const DEMO_SAFE_MAINNET = '0xfF501B324DC6d78dC9F983f140B9211c3EdB4dc7' | ||
|
|
||
| const demoSafeAppsPath = generateSafeRoute(SAFE_ROUTES.APPS, { | ||
| shortName: 'eth', | ||
| safeAddress: DEMO_SAFE_MAINNET, | ||
| }) |
There was a problem hiding this comment.
These should not be here I assume?
There was a problem hiding this comment.
it is our Demo Safe. If the user clicks on Try Demo the shared Safe App should be open with that Demo Safe
There was a problem hiding this comment.
I would move it to the constant file then.
There was a problem hiding this comment.
done!
Added to src/routes/routes.ts
|
|
||
| // if no valid chainId or Safe App is present in query params we redirect to the Welcome page | ||
| useEffect(() => { | ||
| const isValidChainId = safeAppChainId && getChains().find((chain) => chain.chainId === safeAppChainId) |
There was a problem hiding this comment.
You'll need to rename the variable, but we already have isValidChainId() in the config file.
| const isValidChainId = safeAppChainId && getChains().find((chain) => chain.chainId === safeAppChainId) | |
| const isValidChainId = isValidChainId(safeAppChainId) |
| placement="bottom" | ||
| popperOptions={{ positionFixed: true }} | ||
| > | ||
| <ClickAwayListener mouseEvent="onClick" onClickAway={clickAway} touchEvent={false}> |
There was a problem hiding this comment.
onClick is the default prop.
| <ClickAwayListener mouseEvent="onClick" onClickAway={clickAway} touchEvent={false}> | |
| <ClickAwayListener onClickAway={clickAway} touchEvent={false}> |
| </BodyImage> | ||
| {safeAppUrl && ( | ||
| <StyledDemoButton | ||
| data-testid={'open-demo-app-link'} |
There was a problem hiding this comment.
| data-testid={'open-demo-app-link'} | |
| data-testid='open-demo-app-link' |
There was a problem hiding this comment.
As before, please try to select this without using a testing ID.
| const Container = styled.main` | ||
| display: flex; | ||
| justify-content: center; | ||
| margin-top: 20px; | ||
| ` | ||
|
|
||
| const StyledCard = styled(Card)` | ||
| flex-grow: 1; | ||
| max-width: 850px; | ||
| border-radius: 20px; | ||
| padding: 50px 58px; | ||
| ` | ||
|
|
||
| const LoaderContainer = styled.div` | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| min-height: 400px; | ||
| ` | ||
|
|
||
| const DetailsContainer = styled.div` | ||
| display: flex; | ||
| ` | ||
|
|
||
| const SafeIcon = styled.img` | ||
| width: 90px; | ||
| height: 90px; | ||
| ` | ||
|
|
||
| const SafeAppTitle = styled(Title)` | ||
| margin-top: 0px; | ||
| margin-bottom: 12px; | ||
| ` | ||
|
|
||
| const DescriptionContainer = styled.div` | ||
| padding-left: 66px; | ||
| flex-grow: 1; | ||
| ` | ||
|
|
||
| const Separator = styled(Divider)` | ||
| margin: 32px 0; | ||
| ` | ||
|
|
||
| const ChainLabel = styled(Text)` | ||
| color: #b2bbc0; | ||
| ` | ||
|
|
||
| const ChainsContainer = styled.div` | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
|
|
||
| && > div { | ||
| margin-top: 12px; | ||
| margin-right: 8px; | ||
| } | ||
| ` | ||
|
|
||
| const ActionsContainer = styled.div` | ||
| display: flex; | ||
| ` | ||
|
|
||
| const UserSafeContainer = styled.div` | ||
| flex: 1 0 50%; | ||
| text-align: center; | ||
| ` | ||
|
|
||
| const SafeDemoContainer = styled.div` | ||
| flex: 1 0 50%; | ||
| text-align: center; | ||
| ` | ||
| const StyledProvider = styled.div` | ||
| width: 300px; | ||
| height: 56px; | ||
| margin: 0 auto; | ||
| border-radius: 8px; | ||
| border: 2px solid #eeeff0; | ||
| ` | ||
|
|
||
| const BodyImage = styled.div` | ||
| margin: 30px 0; | ||
| ` | ||
|
|
||
| const StyledDemoButton = styled(Button)` | ||
| border: 2px solid #008c73; | ||
| ` |
There was a problem hiding this comment.
A lot of the values here are exported from src/theme/variables.js. Pleaseimport them from there where appropriate instead of declaring them locally..
|
Very excited about this feature, a couple of things I noticed:
|
If the Safe App provided in the url is not present in the Config Service (in this case stage env) we are using the data from the
But If you activate the prod env clicking on the
I am not displaying it because the new designs |
I found it very weird, I started a discussion on the team channel in slack |
src/logic/hooks/useAsync.ts
Outdated
| setIsLoading(false) | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| if (isCurrent) setErr(error) | ||
| if (isCurrent) { | ||
| setError(error) | ||
| setIsLoading(false) |
There was a problem hiding this comment.
I would suggest moving setIsLoading(false) outside of asyncCall(). You won't have to set it twice.
There was a problem hiding this comment.
Not even move it to the finally() block instead?
There was a problem hiding this comment.
at the end we will need something like:
.finally(() => {
if (isCurrent) {
setIsLoading(false)
}
})
But ok, I will update it.
|
|
||
| const SAFE_APP_URL_FROM_CONFIG_SERVICE = 'https://safe-app.gnosis-safe.io/test-safe-app-from-config-service' | ||
| const SAFE_APP_URL_FROM_MANIFEST = 'https://safe-app.gnosis-safe.io/test-safe-app-from-manifest' | ||
| const SAFE_APP_CHAIN_ID = '4' |
There was a problem hiding this comment.
You can use CHAIN_ID.RINKEBY instead of this variable.
| describe('<SafeAppLandingPage>', () => { | ||
| beforeEach(() => { | ||
| // mocking safe apps list from the Config Service endpoint | ||
| jest.spyOn(safeAppsGatewaySDK, 'getSafeApps').mockReturnValue( |
There was a problem hiding this comment.
No need to change this but there is also mockResolvedValue(). You won't need to specify Promise.resolve().
| return getAppInfoFromUrl(safeAppUrl) | ||
| } | ||
|
|
||
| throw 'No Safe App url provided' |
There was a problem hiding this comment.
| throw 'No Safe App url provided' | |
| throw new Error('No Safe App URL provided.') |
| export default SafeAppLandingPage | ||
|
|
||
| const SafeAppDetails = ({ iconUrl, name, description, availableChains }) => { | ||
| const showAvailableChains = availableChains && availableChains.length > 0 |
There was a problem hiding this comment.
| const showAvailableChains = availableChains && availableChains.length > 0 | |
| const showAvailableChains = availableChains?.length > 0 |
| </StyledDemoButton> | ||
| )} | ||
| </SafeDemoContainer> | ||
| ) |
There was a problem hiding this comment.
Thank you for splitting these components. Can you put them in separate folders in the same folder as well?
| const ConnectWallet = () => { | ||
| const { clickAway, open, toggle } = useStateHandler() | ||
|
|
||
| return ( | ||
| <StyledProvider> | ||
| <Provider | ||
| info={<ProviderDisconnected />} | ||
| open={open} | ||
| toggle={toggle} | ||
| render={(providerRef) => | ||
| providerRef.current && ( | ||
| <StyledPopper | ||
| anchorEl={providerRef.current} | ||
| open={open} | ||
| placement="bottom" | ||
| popperOptions={{ positionFixed: true }} | ||
| > | ||
| <ClickAwayListener onClickAway={clickAway} touchEvent={false}> | ||
| <List component="div"> | ||
| <ConnectDetails /> | ||
| </List> | ||
| </ClickAwayListener> | ||
| </StyledPopper> | ||
| ) | ||
| } | ||
| /> | ||
| </StyledProvider> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Can this be reused in the header as well?
There was a problem hiding this comment.
We are already using all components from the header.
There was a problem hiding this comment.
I mean, can you use this component in the header instead of duplicating it?
src/routes/routes.ts
Outdated
| const DEMO_SAFE_MAINNET = '0xfF501B324DC6d78dC9F983f140B9211c3EdB4dc7' | ||
|
|
||
| export const demoSafeRoute = generateSafeRoute(SAFE_ROUTES.APPS, { | ||
| shortName: 'eth', | ||
| safeAddress: DEMO_SAFE_MAINNET, | ||
| }) |
There was a problem hiding this comment.
I would move the DEMO_SAFE_MAINNET to the src/utils/constants file. The demoSafeRoute can stay where it is used. Sorry, perhaps I wasn't clear before.
There was a problem hiding this comment.
For some reasons when I moved the DEMO_SAFE_MAINNET to the src/utils/constants file, some units tests in the non-related googleTagManager.test.tsx file started to fail:
After some research it seems to be related with the way that this mock its defined:
describe('loadGoogleTagManager', () => {
it('prevents init without a gtm id/auth', () => {
jest.doMock('src/utils/constants.ts', () => ({
GOOGLE_TAG_MANAGER_ID: '',
GOOGLE_TAG_MANAGER_DEVELOPMENT_AUTH: '',
}))
[....]
I updated them to only mock the values that are required for that tests using jest.requireActual():
jest.doMock('src/utils/constants.ts', () => {
const original = jest.requireActual('src/utils/constants.ts')
return {
...original,
GOOGLE_TAG_MANAGER_ID: 'id123',
GOOGLE_TAG_MANAGER_DEVELOPMENT_AUTH: 'auth123',
}
})
Now it seems to be working but I think that someone should have a further look into it.
|
Buenos días gentlemen! A couple of notes from my side:
Screen.Recording.2022-04-20.at.10.15.17.mov
We could use the good ol'
(missing bnb chain) (missing the component + all chains) Might there be a conflict because of having 4 chains available on staging only? should be define these differently for dev + staging? Un saludo |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Anotha one, now thinking on the demo feature. Look at this case: Honeyswap is an app that is not available on mainnet but as demos happen on a mainnet safe, the app will open but won't work correctly. Should we do some magic for this cases? Maybe some sort or disclaimer to notify about this cases? Edit: to reproduce it, enable prod CGW beforehand :) |
|
@JagoFigueroa yes, this is something we are aware of. We are still deciding how to best solve this, either creating demostration Safes for each chain or just showing a warning as you mention, because it may happen that we don't know which chains the app is available to offer an alternative |
|
All bueno, gracias guys! |
* Added Share Safe App button * Added unit tests to share safe app button and moved SnackbarProvider component to our Providers file
…app-landing-page # Conflicts: # src/routes/index.tsx # src/routes/routes.ts
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Pull Request Test Coverage Report for Build 2306310096
💛 - Coveralls |
| <SafeDemoContainer> | ||
| <Title size="xs">Want to try the app before using it?</Title> | ||
|
|
||
| <img alt="Demo" height={92} src={DemoSvg} /> |
There was a problem hiding this comment.
WDYT about a styled component here instead setting height attr ?
| // we click on the Share Safe App Button | ||
| fireEvent.click(compoundAppShareBtn) | ||
|
|
||
| const compaundUrl = 'https://cloudflare-ipfs.com/ipfs/QmX31xCdhFDmJzoVG33Y6kJtJ5Ujw8r5EJJBrsp8Fbjm7k' |
* Added new Safe App Card Designs
| const chainId = useSelector(currentChainId) | ||
| const dispatch = useDispatch() | ||
|
|
||
| const safeAddress = extractSafeAddress() |
There was a problem hiding this comment.
@DaniSomoza, this has just made it through QA. useSafeAddress now replaces extractSafeAddress.
What it solves
Resolves safe-global/safe-react-apps#414
Resolves safe-global/safe-react-apps#411
When trying to solve the problem statement of sharing a Safe App via link, we will start creating the landing page which shows the information for a Safe App, as would be the first necessary view to tackle this problem
Figma: https://www.figma.com/file/v56CKWh6Yp60cSsZ71URB0/dApp-sharing-functionality?node-id=1%3A111
How this PR fixes it
A new route is defined for the new
SafeAppLandingPagecomponent:How to test it
Just open a Safe App share link:
Share a Safe App defined in the config service
Tx Builder Mainnet
Tx Builder Rinkeby
Share a Safe App not defined in the config service
Safe App details loaded from the
manifest.jsonScreenshots
Wallet Connected
No Wallet Connected