Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add layout for authenticated pages #3058

Merged
merged 15 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
548 changes: 526 additions & 22 deletions package-lock.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
"express-rate-limit": "^6.7.0",
"express-session": "^1.17.3",
"helmet": "^6.0.0",
"iron-session": "^6.3.1",
"jsdom": "^22.0.0",
"knex": "^2.4.2",
"mozlog": "^3.0.2",
Expand Down
49 changes: 49 additions & 0 deletions public/nextjs_migration/client/js/userMenu.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

const userMenuButton = document.querySelector('.user-menu-button')
const userMenuPopover = document.querySelector('.user-menu-popover')
const userMenuWrapper = document.querySelector('.user-menu-wrapper')

function handleBlur (event, onBlur) {
const currentTarget = event.currentTarget

requestAnimationFrame(() => {
const isChildElement = currentTarget.contains(document.activeElement)

if (!isChildElement) {
onBlur()
}
})
}

function handleMenuButton () {
if (!userMenuPopover || !userMenuWrapper) {
return
}

if (userMenuPopover.hasAttribute('hidden')) {
// Show popover
userMenuPopover.setAttribute('aria-expanded', true)
userMenuPopover.removeAttribute('hidden')

// Handle onblur
userMenuWrapper.addEventListener('blur', (event) => handleBlur(event, handleMenuButton))
userMenuWrapper.focus()

// window.gtag('event', 'opened_closed_user_menu', { action: 'open' })
} else {
// Hide popover
userMenuPopover.setAttribute('aria-expanded', false)
userMenuPopover.setAttribute('hidden', '')

userMenuButton.focus()

// window.gtag('event', 'opened_closed_user_menu', { action: 'close' })
}
}

if (userMenuButton) {
userMenuButton.addEventListener('click', handleMenuButton)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

export default async function UserBreaches() {
return <div>Dashboard</div>;
}
272 changes: 272 additions & 0 deletions src/app/(nextjs_migration)/(authenticated)/user/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,272 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import { ReactNode } from "react";
import Image from "next/image";
import Script from "next/script";

import "../../../../client/css/index.css";
import AppConstants from "../../../../appConstants.js";
import MonitorLogo from "../../../../client/images/monitor-logo-transparent@2x.webp";
import MozillaLogo from "../../../../client/images/moz-logo-1color-white-rgb-01.svg";
import { getL10n } from "../../../functions/server/l10n";
import { getCurrentSession } from "../../../functions/server/sessionHelpers";

export type Props = {
children: ReactNode;
};

const dummyData = {
partial: {
name: "breaches",
},
meta: {
title: "",
socialTitle: "",
socialDescription: "",
},
pathname: "/",
fxaProfile: {
avatar: "https://profile.accounts.firefox.com/v1/avatar/f",
},
};

import RelayLogo from "../../../../client/images/logo-relay.svg";
import VPNLogo from "../../../../client/images/logo-vpn.svg";
import OpenInIcon from "../../../../client/images/icon-open-in.svg";
import SettingsIcon from "../../../../client/images/icon-settings.svg";
import HelpIcon from "../../../../client/images/icon-help.svg";
import SignOutIcon from "../../../../client/images/icon-signout.svg";

const MainLayout = async (props: Props) => {
const session = await getCurrentSession();
const l10n = getL10n();

const isBreachesPage = dummyData.partial.name === "breaches";
const isSettingsPage = dummyData.partial.name === "settings";

const UserMenu = ({ userData }) => {
const fxaUser = userData?.fxa_profile_json;

return (
<div className="user-menu-wrapper" tabIndex={-1}>
<Script src="/nextjs_migration/client/js/userMenu.js" />
<button
aria-expanded="false"
aria-haspopup="true"
className="user-menu-button"
title={l10n.getString("menu-button-title")}
>
<img src={fxaUser?.avatar} alt={l10n.getString("menu-button-alt")} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Should this be a vanilla <img> or Next <Image>?
Also, what's the ?. check in the src attribute? Does that imply that we might not actually have an avatar image and if so, we'd get something like an empty src="" or something like src="undefined" if somehow the fxaUser was falsey?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess follow up question, do we need a width= and/or height= attribute on the image (whichever version we go with) to avoid potential CLS?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have the same question re: Image, one of the benefits to switching in the ADR was to get things like the automatic image optimization without having to do the way we do it now.

If however the approach here is to first convert from existing templates -> jsx with a minimum of changes and then later convert to Next components I think that's a fine approach, but we should be explicit if so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, looks like the only usage of <img /> tag in src/app/(nextjs_migration/** directory:

git grep -n "<img " src/app/\(nextjs_migration\) | cat

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx:61:          <img src={fxaUser?.avatar} alt={l10n.getString("menu-button-alt")} />

It's been a while since I've looked at Next.js linting, so I might need to read up on https://nextjs.org/docs/pages/building-your-application/configuring/eslint and see what the latest recommendations are.

"Well, actually…", it looks like running npx next lint flags this, fascinating!

npx next lint

./src/app/(nextjs_migration)/(authenticated)/user/layout.tsx
61:11  Warning: Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element  @next/next/no-img-element

...

NOTE: And for anybody else odd enough to try getting into piping and grepping next lint output, I had to tweak the following to disable colors (since --no-color was --no-bueno, and they seem to be writing to STDERR instead of STDOUT):

FORCE_COLOR=0 npx next lint &> lint.log

Took a bit of poking around https://github.com/vercel/next.js/blob/canary/packages/next/src/cli/next-lint.ts and https://github.com/chalk/chalk to piece that together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome grep reference!

</button>
<menu
aria-label={l10n.getString("menu-list-accessible-label")}
className="user-menu-container user-menu-popover"
role="navigation"
hidden
>
<li tabIndex={1}>
<a
href={AppConstants.FXA_SETTINGS_URL}
target="_blank"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add rel="noreferrer noopener" to these external links, or is that handled elsewhere in headers, etc?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that noopener is implied when using _blank now: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener - it's not harmful but not necessary in modern browsers to avoid reverse-tabnapping etc. if the FxA server was compromised.

I think we probably do want to send the referrer to FxA though.

className="user-menu-header"
>
<b className="user-menu-email">{fxaUser.email}</b>
<div className="user-menu-subtitle">
{l10n.getString("menu-item-fxa")}
<Image alt="" src={OpenInIcon} />
</div>
</a>
</li>
<hr />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Not sure this is valid. I ran the following snippet through https://validator.w3.org/nu/#textarea and it threw the following error:

Error: Element hr not allowed as child of element menu in this context. (Suppressing further errors from this subtree.)

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>OK, Computer</title>
  </head>

  <body>
    <menu>
      <li>One</li>
      <li>Two</li>
      <hr />
      <li>Four</li>
    </menu>
  </body>
</html>

Copy link
Collaborator Author

@flozia flozia May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Thanks for catching this one. This right now is how we have it on the existing Monitor site — I’ll replace it because invalid syntax is really not optimal. Otherwise, the JSX for the pages is almost one-to-one taken from the existing string literals as a first step to replace the existing architecture with Next.

Seems like moving it does the trick: d98c919.

<li>
<a href="/user/settings" className="user-menu-link">
<Image alt="" src={SettingsIcon} />
{l10n.getString("menu-item-settings")}
</a>
</li>
<li>
<a
href="https://support.mozilla.org/kb/firefox-monitor"
target="_blank"
className="user-menu-link"
>
<Image alt="" src={HelpIcon} />
{l10n.getString("menu-item-help")}
</a>
</li>
<li>
<a href="/user/logout" className="user-menu-link">
<Image alt="" src={SignOutIcon} />
{l10n.getString("menu-item-logout")}
</a>
</li>
</menu>
</div>
);
};

return (
<>
<header>
<a href="/user/breaches">
<Image
className="monitor-logo"
src={MonitorLogo}
width="213"
height="33"
alt={l10n.getString("brand-fx-monitor")}
/>
</a>
<div className="nav-wrapper">
<button className="nav-toggle">
<svg
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 10 8"
width="20"
>
<path
d="M1 1h8M1 4h8M1 7h8"
stroke="#000"
strokeWidth="1"
strokeLinecap="round"
/>
</svg>
</button>
<UserMenu userData={session?.user} />
</div>
</header>

<nav className="site-nav">
<div className="pages-nav">
<a
href="/user/breaches"
className={`nav-item ${isBreachesPage ? "current" : ""}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: not an endorsement one way or the other, but in FxA/SubPlat, we've been using https://www.npmjs.com/package/classnames and it seems to be a nice enhancement.

className={ classNames("nav-item", { current: isBreachesPage }) }

Same w/ L198 below, if we want to add the extra dependency (which might already be in the node_modules anyways):

className={`nav-item ${isSettingsPage ? "current" : ""}`}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like classNames — I’ll bring it up with the other engineers.

>
<svg
width="24"
height="24"
viewBox="0 0 24 24"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
fillRule="evenodd"
clipRule="evenodd"
d="M10.5942 20.049C9.87439 21.3816 10.8394 22.9996 12.3539 22.9996H19.657C21.1692 22.9996 22.1344 21.3862 21.4193 20.0538L17.7796 13.2724C17.0264 11.8689 15.0148 11.8662 14.2577 13.2676L10.5942 20.049Z"
fill="white"
stroke="currentcolor"
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
/>
<path
fillRule="evenodd"
clipRule="evenodd"
d="M16 21C16.5523 21 17 20.5523 17 20C17 19.4477 16.5523 19 16 19C15.4477 19 15 19.4477 15 20C15 20.5523 15.4477 21 16 21Z"
fill="currentcolor"
/>
<path
d="M16 17V16"
stroke="currentcolor"
strokeWidth="2"
strokeLinecap="round"
/>
<path
d="M7 22H5C3.89543 22 3 21.1046 3 20V11C3 9.89543 3.89543 9 5 9H19C20.1046 9 21 9.89543 21 11V13"
stroke="currentcolor"
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
/>
<path
d="M7 9V7C7 4.23858 9.23858 2 12 2C14.7614 2 17 4.23858 17 7V9"
stroke="currentcolor"
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
/>
</svg>
{l10n.getString("site-nav-breaches-link")}
</a>
</div>

<div className="meta-nav">
<a
href="/user/settings"
className={`nav-item ${isSettingsPage ? "current" : ""}`}
>
{l10n.getString("site-nav-settings-link")}
</a>

<a
target="_blank"
href="https://support.mozilla.org/kb/firefox-monitor"
className="nav-item"
>
{l10n.getString("site-nav-help-link")}
</a>
</div>

<div className="callouts">
<p>{l10n.getString("site-nav-ad-callout")}</p>
<a
href="https://relay.firefox.com/?utm_medium=mozilla-websites&utm_source=monitor&utm_campaign=&utm_content=nav-bar-global"
target="_blank"
>
<Image alt={l10n.getString("brand-relay")} src={RelayLogo} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed below (L237) for the Mozilla logo we added loading="lazy" attribute. Not sure if we need that here (and possibly L224 for the VPN logo).

</a>
<a
href="https://www.mozilla.org/products/vpn?utm_medium=mozilla-websites&utm_source=monitor&utm_campaign=&utm_content=nav-bar-global"
target="_blank"
>
<Image alt={l10n.getString("brand-mozilla-vpn")} src={VPNLogo} />
</a>
</div>
</nav>

<main>{props.children}</main>

<footer className="site-footer">
<a href="https://www.mozilla.org" target="_blank">
<Image
src={MozillaLogo}
width="100"
height="29"
loading="lazy"
alt={l10n.getString("mozilla")}
/>
</a>
<menu>
<li>
<a href="/breaches">{l10n.getString("footer-nav-all-breaches")}</a>
</li>
<li>
<a
href="https://support.mozilla.org/kb/firefox-monitor-faq"
target="_blank"
>
FAQ
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that we never submitted this for translation 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was already wondering if “FAQ” is just something that was decided to not be translated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

</a>
</li>
<li>
<a
href="https://www.mozilla.org/privacy/firefox-monitor"
target="_blank"
>
{l10n.getString("terms-and-privacy")}
</a>
</li>
<li>
<a href="https://github.com/mozilla/blurts-server" target="_blank">
{l10n.getString("github")}
</a>
</li>
</menu>
</footer>
</>
);
};

export default MainLayout;
Loading