-
Notifications
You must be signed in to change notification settings - Fork 362
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
return ( | ||
<> | ||
{featuredApps.map((app) => { | ||
const appRoute = generateSafeRoute(SAFE_ROUTES.APPS, routesSlug) + `?appUrl=${app.url}` |
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 you extract this to a function? It will probably use useful in the other Safe Apps 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.
done
` | ||
|
||
// TODO: Replace once tags are available | ||
const featuredAppIds = ['29', '11'] |
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.
Tags are available on staging. Can we use them here?
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.
Good point, I switched over to filter by tags and removed the hard-coded ids. We still need to update the type in the gateway sdk. For the time being I extended the type here.
` | ||
|
||
// TODO: Replace with type from gateway-sdk once available | ||
type SafeAppWithTags = SafeApp & { tags: string[] } |
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's nice to see you extend the type here but we should do this in the gateway types first.
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.
Just saw that its already there in v2.10.3. Will update the dependency and remove this type.
const { allApps } = useAppList() | ||
const { address } = useSelector(currentSafe) ?? {} | ||
const featuredApps = useMemo( | ||
() => allApps.filter((app) => (app as SafeAppWithTags).tags?.includes('dashboard-widgets')), |
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.
By updating the gateway tiypes, you won't need to cast this.
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.
LGTM!
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!
800f61d
to
c768834
Compare
c768834
to
e17b054
Compare
What it solves
Part of #3693
How this PR fixes it
Adds a widget to display featured apps in the dashboard. Layout will be done in a separate PR.
How to test it