sts-sponsors team mailing list archive

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

Review: Approve code, qa

LGTM, just one comment concerning aria labels for the header and nav.

Diff comments:

> diff --git a/frontend/src/components/Navigation/Navigation.tsx b/frontend/src/components/Navigation/Navigation.tsx
> index f2b3a13..1849bba 100644
> --- a/frontend/src/components/Navigation/Navigation.tsx
> +++ b/frontend/src/components/Navigation/Navigation.tsx
> @@ -33,7 +33,7 @@ const Navigation = (): JSX.Element => {
>  
>    return (
>      <>
> -      <header className="l-navigation-bar">
> +      <header aria-label="navigation" className="l-navigation-bar">

I'm not 100% with these aria labels - the semantic elements *probably* do enough to differentiate the header from the side nav, but I also feel as though labelling the header as "navigation" is a bit misleading, since it only really serves as a mean to open the actual navigation, rather than being a navigation element in and of itself.

Up to you whether or not to change this though

>          <div className="p-panel is-dark">
>            <div className="p-panel__header">
>              <NavigationBanner />


-- 
https://code.launchpad.net/~petermakowski/maas-site-manager/+git/site-manager/+merge/439146
Your team MAAS Committers is subscribed to branch ~petermakowski/maas-site-manager:add-side-panel-MAASENG-1487.