feat: introduce temporary admin mode for site config #1033
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 261 Passed, 36 Skipped, 47.08s Total Time |
dcshzj
force-pushed
the
feat/site-config-admin
branch
from
ba63312 to
75b5952
Compare
dcshzj
force-pushed
the
feat/site-config-admin
branch
from
75b5952 to
253b257
Compare
There was a problem hiding this comment.
minor issue - not too sure what the useEffect is doing! i think because this affects end user sites (and the site wide config), we might want to sanitise the json object to make sure it's valid (cos we're just parsing to make sure it's valid json, but not that the jsonobject matches what we expect - eg: {siteNavItems: [...]} vs {siteNavItem: [...]}
but no actionable re: the second point of matching - onlyi want to make sure the useEffect isn't sussy
| useEffect(() => { | ||
| const handleRouteChange = (url: string) => { | ||
| if (isDirty) { | ||
| router.events.off("routeChangeStart", handleRouteChange) | ||
| setNextUrl(url) | ||
| router.events.emit("routeChangeError") | ||
| // eslint-disable-next-line @typescript-eslint/only-throw-error | ||
| throw "Error to abort router route change. Ignore this!" | ||
| } | ||
| } | ||
|
|
||
| if (!isOpen) { | ||
| router.events.on("routeChangeStart", handleRouteChange) | ||
| } | ||
| return () => { | ||
| router.events.off("routeChangeStart", handleRouteChange) | ||
| } | ||
| }, [isOpen, router.events, isDirty]) |
There was a problem hiding this comment.
i'm not quiet sure what this useEffect does? if the modal is closed + there are changes, we unsub to routeChangeStart (so this iwll only ever fire once) -> and we set the next url
why wouldn't we use the nextUrl of the unsaved settings modal
There was a problem hiding this comment.
Hmm this was taken directly from the settings.tsx file, mainly to trigger the unsaved settings modal if the form is dirty and the user is attempting to change to a different route (e.g. clicking on the pages icon to go back to the site dashboard). This useEffect will prevent that.
There was a problem hiding this comment.
ok, sounds good! this honestly look abit sus but given that it's used on settings.tsx , i think it's ok.
i think it might be worth to double-check the function is cleaning up properly; just based off what i see here, we turn off the events if isDirty then we setNextUrl, but we only turn on the event when the modal is closed again? wondering if it's possible for our users to bypass the !isOpen and for routeChangeStart to be off permanently
based off what i know i don't think so, because any closing of the modal will trigger the state change, but just wanted to be sure
| @@ -136,4 +144,51 @@ export const siteRouter = router({ | |||
|
|
|||
| return input | |||
| }), | |||
| setSiteConfigAdmin: protectedProcedure | |||
There was a problem hiding this comment.
nit: i think this might be better named as setSiteConfigByAdmin or setAdminSiteConfig but very minor
Problem
Ops load on engineers is quite high for editing the site config, navbar and footer.
Solution
Breaking Changes
Features:
/adminroute for Isomer Admins to directly modify the site's config, theme, navbar and footer items.Before & After Screenshots
View for Isomer Admins:
View for unauthorised users:
Tests
/adminto the URL and navigate to the page.