Review: Approve
This is good to merge IMO. One comment below but this is optional. Might also
be worth writing an E2E test for the secondary nav behaviour but I'll leave
that up to you
Diff comments:
> diff --git a/frontend/src/routes/routes.tsx b/frontend/src/routes/routes.tsx
> index 8b6e071..466ac30 100644
> --- a/frontend/src/routes/routes.tsx
> +++ b/frontend/src/routes/routes.tsx
> @@ -34,19 +35,29 @@ export const routes = createRoutesFromElements(
> <Route
> element={
> <RequireLogin>
> - <Requests />
> + <Settings />
> </RequireLogin>
> }
> - path="requests"
> - />
> - <Route
> - element={
> - <RequireLogin>
> - <Tokens />
> - </RequireLogin>
> - }
> - path="tokens"
> - />
> + path="settings"
> + >
> + <Route element={<RequireLogin />} index loader={() =>
> redirect("tokens")} />
> + <Route
> + element={
> + <RequireLogin>
> + <Tokens />
> + </RequireLogin>
> + }
> + path="tokens"
> + />
> + <Route
> + element={
> + <RequireLogin>
> + <Requests />
> + </RequireLogin>
> + }
> + path="requests"
> + />
> + </Route>
You don't have to do this now, but for the future it might be worth creating an
extension of the Route component that automatically wraps the child element in
RequireLogin, maybe something like AuthRoute for a name
> <Route path="users" />
> </Route>,
> );
--
https://code.launchpad.net/~petermakowski/maas-site-manager/+git/site-manager/+merge/441449
Your team MAAS Committers is subscribed to branch
~petermakowski/maas-site-manager:settings-subnavigation-MAASENG-1508.
--
Mailing list: https://launchpad.net/~sts-sponsors
Post to : [email protected]
Unsubscribe : https://launchpad.net/~sts-sponsors
More help : https://help.launchpad.net/ListHelp
