sts-sponsors team mailing list archive

[Nick De Villiers <mp+441449@xxxxxxxxxxxxxxxxxx>]

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.