-
Notifications
You must be signed in to change notification settings - Fork 509
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: Dashboard governance widgets #1149
Conversation
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Deploying with
|
Latest commit: |
533a4c3
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6cbf0990.web-core.pages.dev |
Branch Preview URL: | https://dashboard-mini-safe-apps.web-core.pages.dev |
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.
Looking good. I've noticed a few issues though:
- On initial load "Explore Safe App" flashes, then the loading message and finally the section closes.
- "Explore Safe Apps" shouldn't show.
- Neither it or the loading message should flash.
- I think the
Accordion
should then be open by default and we save the expansion preference locally.
- The Claiming App widget does not load.
- The truncation of proposal names doesn't work in the
iframe
.
</Box> | ||
</Card> | ||
if (!claimingApp) { | ||
return null |
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 should give some feedback instead of showing nothing.
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.
Can we write "There was an error fetching the widgets" ?
Note: this is for an error on the following request https://safe-client.staging.5afe.dev/v1/chains/1/safe-apps?client_url=http://localhost:3000
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 an error fetching the widget" works.
This default resulted from https://www.notion.so/gnosis-safe/Kickoff-SafeDAO-27-10-2022-2234309ec4ba47ce87ccf662c867aa8c Can you confirm @liliiaorlenko ? Should the Governance section start collapsed or expanded? |
If that's the intended design then let's stick to it but the loading text shouldn't show unless it's open. |
It should start open. Let me know when I can recommend suggestions for implemented visual improvements, there is some spacing fixes I would like to add |
@liliiaorlenko now it is a good time for design remarks 🎨 you can access it here https://dashboard-mini-safe-apps.web-core.pages.dev/ |
const { getAllowedFeaturesList } = useBrowserPermissions() | ||
const [claimingSafeApp, errorFetchingClaimingSafeApp] = useRemoteSafeApps(SafeAppsTag.SAFE_CLAIMING_APP) | ||
const claimingApp = claimingSafeApp?.[0] | ||
const fetchingSafeClaimingApp = !claimingApp && !errorFetchingClaimingSafeApp |
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've implemented this loading logic because the loading state returned by useRemoteSafeApps
starts 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.
Can we not move this logic into the hook? This will forever load if an error occurs.
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.
The truncation looks good now. However, the loading screen still flashes before closing the Accordion
. As suggested, it should load open. I would suggest we then cache the open preference locally.
const { getAllowedFeaturesList } = useBrowserPermissions() | ||
const [claimingSafeApp, errorFetchingClaimingSafeApp] = useRemoteSafeApps(SafeAppsTag.SAFE_CLAIMING_APP) | ||
const claimingApp = claimingSafeApp?.[0] | ||
const fetchingSafeClaimingApp = !claimingApp && !errorFetchingClaimingSafeApp |
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.
Can we not move this logic into the hook? This will forever load if an error occurs.
<Paper> | ||
<Box sx={{ height: '300px' }} display="flex" alignItems="center" justifyContent="center"> | ||
<Typography variant="h1" color="text.secondary"> | ||
Loading Safe Claiming App... |
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 know that they both hypothetically the Claiming App but I think it would look visually better if we split it into the same layout as the widgets, e.g. [ Loading Snapshot ] [ Loading Claiming ]. What do you think? cc @liliiaorlenko
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.
In this case we are just fetching the remote Safe apps endpoint. Each of the widgets loads with its own Skeleton component. So this "loading" component was more for the whole Governance section
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 would adjust it to reference "governance" instead of the Claiming App specifically then.
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.
Can it display "Loading apps" ?
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.
That's quite generic imo. If you wanted to specify apps, I would say splitting the loading section in two as my suggestion further up would make sense.
<Card sx={{ height: '300px' }}> | ||
<SafeAppsErrorBoundary render={() => <SafeAppsLoadError onBackToApps={() => {}} />}> | ||
<AppFrame | ||
appUrl={`${claimingApp.url}#snapshot-widget`} |
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 suggest we move both hashes to constants.
I added the |
After revisiting it, the hook is returning correctly IMO. This is more of a component state logic so I would leave it in the component. Maybe I can rename the variable to |
It's not regarding the naming, it's that both the error and loading flags are referenced. If an error occurs, the UI will show them as persistently loading. We shouldn't combine them. |
eade19c
to
5b3f681
Compare
1f63f11
to
660f56d
Compare
Branch preview |
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 Safe Web Core Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
* create the Governance section on Dashboard * [Governance Visibility] Dashboard section (#1125) * dashboard section with placeholders * use MUI Accordion for the collapsible section * change expand icon color * feat: Dashboard governance widgets (#1149) * feat: load a mini safe app in dashboard's Governance section * fix card heights * display snapshot widget * fetch the claiming app and display the widgets * fix: accordion defaultExpanded and fix loading states flicker * responsive widgets grid * expandDefault true * no need to pass explicit boolean value. improve loading components * feat: pass dark mode in the widgets url (#1219) * expandDefault true * no need to pass explicit boolean value. improve loading components * feat: pass dark mode in the widgets url * pass preferred them in a search parameter * feat: One governance widget (#1237) * feat: call only one widget from the safe-claiming app * rm unused Grid * use widgetWrapper for the loading component * add a custom fallback component * ammend load error copy * use different color in the placeholder as per design * feat: dark flag in URL hash (#1251) * fix: governance widgets CSS (#1254) * fix: Accordion only clickable on expandIcon * adjust AppFrame height for widgets * change the widget load error component * fix code style * improve classnames params * fix: return `null` if no Claiming App on chain (#1284) * fix: return `null` if no Claiming App on chain * fix: reference deployed token address * fix: add address of token on Goerli * fix: add loading wrapper * fix: rename wrapper * Fix: use a non-tracking version of the AppFrame (#1307) * Fix: use a non-tracking version of the AppFrame * Clear communicator handlers * useAppIsLoading * Rename components * Extract getSafeInfo * AppFrameWrapperProps -> AppFrameProps * Fix: reload widget on safe change + catch errors (#1319) Co-authored-by: iamacook <[email protected]> Co-authored-by: katspaugh <[email protected]> Co-authored-by: katspaugh <[email protected]>
What it solves
Mini Safe apps iframes are displayed in the dashboard
Resolves #974
Screenshots