feat(arcgis): auth slice #332
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| <AuthContainer id="dashboard-container" layout={layout}> | ||
| </AuthContainer> |
There was a problem hiding this comment.
| <AuthContainer id="dashboard-container" layout={layout}> | |
| </AuthContainer> | |
| <AuthContainer id="auth-container" layout={layout} /> |
| @@ -0,0 +1,118 @@ | |||
| import styled, { useTheme } from "styled-components"; | |||
There was a problem hiding this comment.
Add unit tests for the file
| </IconContainer> | ||
| <ContentContainer> | ||
| <Title>{title}</Title> | ||
| {typeof content === "function" ? content() : content} |
There was a problem hiding this comment.
I would use "children" for the custom content. content() option is not clear. It is an unnecessary complexity on my opinion. It is always possible to call it in the parent component if needed.
| <AuthContainer id="dashboard-container" layout={layout}> | ||
| </AuthContainer> |
| @@ -0,0 +1,43 @@ | |||
| import { createSlice, createAsyncThunk } from "@reduxjs/toolkit"; | |||
src/utils/arcgis-auth.spec.ts
Outdated
| OLD_ENV = process.env; | ||
| }); | ||
|
|
||
| describe("getAuthenticatedUser", () => { |
There was a problem hiding this comment.
The test suite is testing "getAuthenticatedUser", "arcGisRequestLogin" and "arcGisRequestLogout", but the describe is about only getAuthenticatedUser
src/utils/arcgis-auth.spec.ts
Outdated
| expect(email).toEqual(mockEmailExpected); | ||
| }); | ||
|
|
||
| it("Should return session", async () => { |
There was a problem hiding this comment.
The description is about "getAuthenticatedUser Should return session" but testing getAuthenticatedUser instead
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| background: #00000099; |
There was a problem hiding this comment.
By figma:
#232430 50% . Use global constants for colors.
| display: flex; | ||
| flex-direction: column; | ||
| border-radius: 8px; | ||
| background: ${({ theme }) => theme.colors.mainColor}; |
There was a problem hiding this comment.
This background is not by Figma (white theme).
| const theme = useTheme(); | ||
|
|
||
| return ( | ||
| <Overlay> |
There was a problem hiding this comment.
Overlay trasparency is applied on child components. We cannot use this approach. Instead, we should create another WrapperContainer with position: fixed and so on with visibility: hidden. We set visibility: visible for the child and it will be shown in the center.
There was a problem hiding this comment.
The version that is submitted uses this approach, but actually it doesn't work. The problem is that the "ModalDialog" component is a child of some other component that uses opaque. It's out of my control.
I think we have to use the Popover that renders the dialog in the body, out of any other components.
| const onCancel = jest.fn(); | ||
| const onConfirm = jest.fn(); | ||
|
|
||
| const callRender = (renderFunc, props = {}, store = setupStore()) => { |
| <Overlay /> | ||
| <WrapperContainer> | ||
| <Container data-testid="modal-dialog-content"> | ||
| <IconContainer> |
There was a problem hiding this comment.
could you add cursor: pointer for the close button?
No description provided.