[Issue #2962] Login modal #3433
Conversation
acouch
requested review from
andycochran,
btabaska and
doug-s-nava
as code owners
| if (!environment.AUTH_LOGIN_URL) { | ||
| throw new Error("AUTH_LOGIN_URL not defined"); | ||
| } | ||
| return NextResponse.redirect(environment.AUTH_LOGIN_URL); |
There was a problem hiding this comment.
Returning NextResponse.redirect instead of redirect doesn't throw an error for the try/catch block.
| } | ||
| return NextResponse.redirect(environment.AUTH_LOGIN_URL); | ||
| } catch (error) { | ||
| return new NextResponse(error as string, { status: 500 }); |
There was a problem hiding this comment.
Not sure if we should have a more graceful error message, but this is temporary anyway.
| @@ -380,7 +380,7 @@ button.usa-pagination__button.usa-button { | |||
| } | |||
|
|
|||
| @include at-media("desktop") { | |||
| .desktop\:margin-bottom-5px { | |||
| margin-bottom: 5px !important; | |||
| .desktop\:margin-bottom-3px { | |||
There was a problem hiding this comment.
USWDS has margins of 1px, 2px, or 4px:
acouch
force-pushed
the
acouch/issue-2962-sign-in-modal
branch
from
3523540 to
0bf5664
Compare
| loginUrl={authLoginUrl || ""} | ||
| /> | ||
| )} | ||
| {!user?.token && <LoginModal navLoginLinkText={t("nav_link_login")} />} |
There was a problem hiding this comment.
this works because the LoginModal component includes the modal control button but if you're expecting to see something like <LoginButton> here, it's a little confusing to see <LoginModal> instead. Something for later, but there's probably a refactor that would make this a little easier to understand
| }), | ||
| })); | ||
|
|
||
| describe("LoginModal", () => { |
There was a problem hiding this comment.
can we add a test that the login button directs to the correct location?
There was a problem hiding this comment.
This is a constant now, so it would be just checking the link. Can add in a follow up
Summary
Fixes #2962
Time to review: 15 mins
Changes proposed
Mobile
Desktop
TODO