-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #2654] Next login flow backend #3182
Conversation
b596f57
to
79c1d6f
Compare
@@ -24,7 +24,7 @@ export default function NotFound() { | |||
<> | |||
<BetaAlert /> | |||
<GridContainer className="padding-y-1 tablet:padding-y-3 desktop-lg:padding-y-15 measure-2"> | |||
<h1 className="nj-h1">{t("title")}</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like maybe a remnant from the nj ui project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
@@ -94,3 +94,6 @@ export const findFirstWhitespace = (content: string, startAt: number): number => | |||
// is used by Next middleware throws a compilation error, so let's roll our own | |||
export const camelToSnake = (camel: string) => | |||
camel.replace(/[A-Z]/g, (letter) => `_${letter}`); | |||
|
|||
export const encodeText = (valueToEncode: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved here to ease testing so we can avoid mocking the window
329a805
to
7ee37c5
Compare
@@ -13,12 +13,6 @@ import SearchBar from "src/components/search/SearchBar"; | |||
import SearchFilters from "src/components/search/SearchFilters"; | |||
import ServerErrorAlert from "src/components/ServerErrorAlert"; | |||
|
|||
interface ErrorProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I removed the error page for User, this doesn't need to be moved at this point, but it's still good practice, I think, so we have it more widely available for future error pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to look a little more at error handling across the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Summary
Fixes #2654
Time to review: 20 mins
Changes proposed
This is step 1 for implementing auth support on the Next JS application.
The change includes:
Context for reviewers
Test steps
npm run dev
New Login
created session
messageExisting Login
already logged in
messageError Cases
no token provided
messageAdditional information
An example of a client side session cookie