-
Notifications
You must be signed in to change notification settings - Fork 408
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
Fix: Safe Apps loading state #1674
Conversation
Branch preview✅ Deploy successful! |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
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.
Tested and looks good. Only thing I 'd suggest to adjust is this skeleton in the NftGrid
to match the checkbox width
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.
👍
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.
src/pages/apps/index.tsx
Outdated
@@ -21,8 +21,8 @@ import { getOrigin } from '@/components/safe-apps/utils' | |||
const SafeApps: NextPage = () => { | |||
const chainId = useChainId() | |||
const router = useRouter() | |||
|
|||
const [appUrl, routerReady] = useSafeAppUrl() |
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.
It seems like we shouldn't have been using router.isReady
for conditional rendering at all.
Co-authored-by: Aaron Cook <aaron@safe.global>
That's why I originally had a circular loader there but now we optimize for the happy path, i.e. when you do have NFTs. One can argue that if a person deliberately visits that page, they likely have NFTs. |
Reg. showing an empty assets table, it probably was a bad idea to allow hiding the native token (I insisted on it), but it's an edge case. |
I accidentally merged this branch to dev prior to the last two commits. So I'll merge the rest as well, it fixes the e2e tests. |
d02980d
to
98d06e6
Compare
Custom app test Create a separate route for individual apps Tests
What it solves
Resolves #1671
How this PR fixes it
I've created a separate route for individual Safe Apps:
/apps/open?appUrl=...
.The Safe Apps list will remain on
/apps
.This makes it easier to pre-render the route in an efficient way, and show placeholder skeletons.
How to test it
Go to apps, open an app, reload the page.
Test custom apps.