sts-sponsors team mailing list archive
-
sts-sponsors team
-
Mailing list archive
-
Message #07982
Re: [Merge] ~jonesogolo/maas-site-manager:1561-update-mobile-menu into maas-site-manager:main
one first idea inline
Diff comments:
> diff --git a/frontend/src/components/Navigation/Navigation.tsx b/frontend/src/components/Navigation/Navigation.tsx
> index 701e93c..1b39a24 100644
> --- a/frontend/src/components/Navigation/Navigation.tsx
> +++ b/frontend/src/components/Navigation/Navigation.tsx
> @@ -37,6 +37,8 @@ type NavProps = {
> isLoggedIn: boolean;
> };
>
> +const MOBILE_BREAKPOINT_WIDTH = 460;
It feels like breakpoints should go to some central place so we can easily reuse them in different components. Also 460 seems a bit on the low end. See e.g. https://www.browserstack.com/guide/responsive-design-breakpoints or https://bulma.io/documentation/customize/variables/ for some best practices in breakpoint definitions.
> +
> const Navigation = ({ isLoggedIn }: NavProps): JSX.Element => {
> const [isCollapsed, setIsCollapsed] = useLocalStorageState<boolean>("appSideNavIsCollapsed", { defaultValue: true });
> const location = useLocation();
--
https://code.launchpad.net/~jonesogolo/maas-site-manager/+git/maas-site-manager/+merge/442211
Your team MAAS Committers is requested to review the proposed merge of ~jonesogolo/maas-site-manager:1561-update-mobile-menu into maas-site-manager:main.
References