Add upload favicon settings section #6638
Conversation
|
Looks good to me. A super super tiny nit is that the default Rill image in the upload section is our logo and actually not the favicon but not even sure that's worth changing |
| const assetCreator = createAdminServiceCreateAsset(); | ||
| const orgUpdater = createAdminServiceUpdateOrganization(); |
There was a problem hiding this comment.
Missing loading & error handling on the orgUpdater request
| } | ||
|
|
||
| async function onSave() { | ||
| onCancel(); |
There was a problem hiding this comment.
I'd expect the onCancel() (or a simple closePopover()) call to happen after the mutation, not before. Then we we can handle loading & error states in the popover itself.
(And the same goes for the onCancel() call in the removeLogo() function.)
| void queryClient.invalidateQueries( | ||
| getAdminServiceGetOrganizationQueryKey(organization), | ||
| ); | ||
| void invalidateAll(); |
There was a problem hiding this comment.
Do we need to invalidateAll() or can we be more precise with invalidate()?
AdityaHegde
force-pushed
the
feat/favicon-upload-frontend
branch
from
e84823f to
256ce28
Compare
web-admin/src/routes/+layout.ts
Outdated
| @@ -23,7 +23,8 @@ import { fixLocalhostRuntimePort } from "@rilldata/web-common/runtime-client/fix | |||
| import { runtime } from "@rilldata/web-common/runtime-client/runtime-store"; | |||
| import { error, redirect, type Page } from "@sveltejs/kit"; | |||
|
|
|||
| export const load = async ({ params, url, route }) => { | |||
| export const load = async ({ params, url, route, depends }) => { | |||
| depends("init"); | |||
There was a problem hiding this comment.
Can we call this "root", just so we don't confuse it with Rill Developer's "init" loader (which is called "init" because it has "initialization" logic that redirects users to the Welcome page)
|
|
||
| function onCancel() { | ||
| open = false; | ||
| url = imageUrl; |
There was a problem hiding this comment.
Just checking – is this needed? I see the reactive statement above ($: url = imageUrl;). Regardless, it's probably worth clarifying with a comment why the url variable is needed in addition to imageUrl.
There was a problem hiding this comment.
Adding a comment to clarify why we need it.
closes #6600
Builds on top of #6606 and adds a settings section for favicon upload.
Checklist: