fix dev review issues

visgl · Dec 7, 2023 · 0c5ffee · 0c5ffee
1 parent 2fa7c09
commit 0c5ffee
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 96 deletions.
diff --git a/public/icons/logout.svg b/public/icons/logout.svg
diff --git a/src/app.tsx b/src/app.tsx
@@ -20,6 +20,7 @@ import {
   color_accent_primary,
   color_accent_tertiary,
   color_brand_tertiary,
+  dim_brand_tertinary
 } from "./constants/colors";
 import * as Pages from "./pages";
 import { AppThemes, ComparisonMode, Theme } from "./types";
@@ -79,13 +80,11 @@ const THEMES: AppThemes = {
 
       actionIconButtonDisabledColor: color_canvas_primary,
       actionIconButtonDisabledBG: dim_canvas_primary,
-      actionIconButtonDisabledBGHover: `${dim_canvas_primary}99`,// TODO: waiting for the design...
       actionIconButtonTextDisabledColor: dim_canvas_primary,
+      actionIconButtonTextDisabledColorHover: dim_brand_tertinary,
 
-      actionIconButtonActiveColor: color_brand_tertiary,
-      actionIconButtonActiveBG: `${color_brand_tertiary}66`,
-      actionIconButtonActiveBGHover: `${color_brand_tertiary}33`,
-      actionIconButtonTextActiveColor: color_brand_tertiary,
+      logoutButtonTextColor: dim_canvas_primary,
+      logoutButtonIconColorHover: dim_brand_tertinary,
 
       esriImageColor: color_canvas_secondary_inverted,
     },
@@ -122,13 +121,11 @@ const THEMES: AppThemes = {
 
       actionIconButtonDisabledColor: color_canvas_secondary,
       actionIconButtonDisabledBG: `${color_brand_tertiary}66`,
-      actionIconButtonDisabledBGHover: `${color_brand_tertiary}33`,// TODO: waiting for the design...
       actionIconButtonTextDisabledColor: `${color_brand_tertiary}66`,
+      actionIconButtonTextDisabledColorHover: color_brand_tertiary,
 
-      actionIconButtonActiveColor: color_brand_tertiary,
-      actionIconButtonActiveBG: `${color_brand_tertiary}66`,
-      actionIconButtonActiveBGHover: `${color_brand_tertiary}33`,
-      actionIconButtonTextActiveColor: color_brand_tertiary,
+      logoutButtonTextColor: color_brand_quaternary,
+      logoutButtonIconColorHover: color_brand_tertiary,
 
       esriImageColor: `${color_brand_tertiary}66`,
     },

diff --git a/src/components/action-icon-button/action-icon-button.spec.tsx b/src/components/action-icon-button/action-icon-button.spec.tsx
@@ -10,9 +10,9 @@ const onClickMock = jest.fn();
 const callRender = (renderFunc, props = {}) => {
   return renderFunc(
     <ActionIconButton
-      icon={PlusIcon}
+      Icon={PlusIcon}
       style='disabled'
-      buttonSize={ButtonSize.Small}
+      size={ButtonSize.Small}
       onClick={onClickMock}
       {...props} />
   );
@@ -31,7 +31,7 @@ describe("Plus Button", () => {
   });
 
   it("Should render Big Plus button", () => {
-    const { container } = callRender(renderWithTheme, { children: 'Test Button', buttonSize: ButtonSize.Big });
+    const { container } = callRender(renderWithTheme, { children: 'Test Button', size: ButtonSize.Big });
     expect(container).toBeInTheDocument();
     const button = screen.getByText('Test Button');
     const buttonHeight = getComputedStyle(button.previousSibling as Element).getPropertyValue("height");

diff --git a/src/components/action-icon-button/action-icon-button.tsx b/src/components/action-icon-button/action-icon-button.tsx
@@ -1,51 +1,45 @@
-import React from "react";
+import React, { useMemo } from "react";
 import styled from "styled-components";
 import { ButtonSize } from "../../types";
 import { StyledComponent, DefaultTheme } from "styled-components";
 
-type ActionIconButtonProps = {
-  children?: React.ReactNode;
-  icon: StyledComponent<any, DefaultTheme, object, string | number | symbol>;
-  buttonSize: ButtonSize;
-  style?: "active" | "disabled";
-  onClick?: () => void;
-};
-
-/*
-  https://developer.mozilla.org/en-US/docs/Web/CSS/opacity
-
-  opacity applies to the element as a whole, including its contents, even though the value is not inherited by child elements.
-  Thus, the element and its children all have the same opacity relative to the element's background,
-  even if they have different opacities relative to one another.
-
-  To change the opacity of a background only, use the background property with a color value that allows for an alpha channel. F
-*/
+import {
+  color_brand_tertiary,
+} from "../../constants/colors";
 
 const Button = styled.div<{ grayed?: boolean }>`
   display: flex;
-  cursor: pointer;
   justify-content: flex-start;
   align-items: center;
-  margin-right: 16px;
+  padding: 10px;
+  cursor: pointer;
 
   &:hover {
-    > * {
-      &:first-child {
-        background: ${({ theme, grayed }) => (
-    grayed
-      ? theme.colors.actionIconButtonDisabledBGHover
-      : theme.colors.actionIconButtonActiveBGHover
-  )};
+    > :first-child {
+      background: ${color_brand_tertiary}33;
+
+      > * {
+        fill: ${color_brand_tertiary};
       }
     }
+
+    > :nth-child(2) {
+      color: ${({ theme, grayed }) => (
+        grayed
+          ? theme.colors.actionIconButtonTextDisabledColorHover
+          : color_brand_tertiary
+      )};
+    }
   }
 `;
 
 const ButtonText = styled.div<{ grayed?: boolean }>`
+  position: relative;
+  margin-left: 16px;
   color: ${({ theme, grayed }) => (
     grayed
       ? theme.colors.actionIconButtonTextDisabledColor
-      : theme.colors.actionIconButtonTextActiveColor
+      : color_brand_tertiary
   )};
   `;
 
@@ -58,34 +52,41 @@ const IconContainer = styled.div<{ buttonSize: number, grayed?: boolean }>`
   background: ${({ theme, grayed }) => (
     grayed
       ? theme.colors.actionIconButtonDisabledBG
-      : theme.colors.actionIconButtonActiveBG
+      : `${color_brand_tertiary}66`
   )};
 
   border-radius: 4px;
   align-items: center;
   justify-content: center;
-  margin-right: 16px;
   `;
 
-export const ActionIconButton = (props: ActionIconButtonProps) => {
+type ActionIconButtonProps = {
+  children?: React.ReactNode;
+  Icon: StyledComponent<any, DefaultTheme, object, string | number | symbol>;
+  size: ButtonSize;
+  style?: "active" | "disabled";
+  onClick?: () => void;
+};
+
+export const ActionIconButton = ({ Icon, style, size, children, onClick }: ActionIconButtonProps) => {
+
+  const grayed = useMemo(() => style === "disabled", [style]);
 
-  const grayed = props.style === "disabled";
-  const StyledIcon = styled(props.icon) <{ grayed?: boolean }>`
+  const StyledIcon = styled(Icon) <{ grayed?: boolean }>`
     position: absolute;
     fill: ${({ theme, grayed }) => (
       grayed
         ? theme.colors.actionIconButtonDisabledColor
-        : theme.colors.actionIconButtonActiveColor
+        : color_brand_tertiary
     )};
     `;
 
   return (
-    <Button onClick={props.onClick} grayed={grayed}>
-      <IconContainer buttonSize={props.buttonSize} grayed={grayed}>
-        <props.icon />
+    <Button onClick={onClick} grayed={grayed}>
+      <IconContainer buttonSize={size} grayed={grayed}>
         <StyledIcon grayed={grayed} />
       </IconContainer>
-      <ButtonText grayed={grayed}>{props.children}</ButtonText>
+      <ButtonText grayed={grayed}>{children}</ButtonText>
     </Button>
   );
 };

diff --git a/src/components/layers-panel/layers-control-panel.tsx b/src/components/layers-panel/layers-control-panel.tsx
@@ -35,6 +35,7 @@ const EsriStyledImage = styled(EsriImage)`
   top: 0.37px;
   width: 41.9px;
   height: 15.65px;
+  margin-left: 16px;
   fill: ${({ theme }) => theme.colors.esriImageColor};
 `;
 
@@ -49,17 +50,14 @@ const LayersList = styled.div`
   display: flex;
   flex-direction: column;
   width: 100%;
-  margin-bottom: 10px;
 `;
 
 const InsertButtons = styled.div`
   display: flex;
   flex-direction: column;
-  padding-left: 10px;
+
   > * {
-    &:first-child {
-      margin-bottom: 28px;
-    }
+      margin-bottom: 8px;
   }
 `;
 
@@ -71,12 +69,11 @@ const ChildrenContainer = styled.div`
   padding-left: 12px;
 `;
 
-const ActionIconButtonContainer = styled.div<{ bottom?: number }>`
+const ActionIconButtonContainer = styled.div`
   display: flex;
   flex-direction: row;
   justify-content: start;
   align-items: center;
-  margin-bottom: ${({ bottom = 0 }) => `${bottom}px`};
 `;
 
 export const LayersControlPanel = ({
@@ -211,26 +208,26 @@ export const LayersControlPanel = ({
     <LayersContainer>
       <LayersList>{renderLayers(layers)}</LayersList>
       <InsertButtons>
-        <ActionIconButton icon={PlusIcon} buttonSize={ButtonSize.Small} onClick={onLayerInsertClick}>
+        <ActionIconButton Icon={PlusIcon} size={ButtonSize.Small} onClick={onLayerInsertClick}>
           Insert layer
         </ActionIconButton>
-        <ActionIconButton icon={PlusIcon} buttonSize={ButtonSize.Small} onClick={onSceneInsertClick}>
+        <ActionIconButton Icon={PlusIcon} size={ButtonSize.Small} onClick={onSceneInsertClick}>
           Insert scene
         </ActionIconButton>
 
         <PanelHorizontalLine />
 
         {showLogin && (
           <ActionIconButtonContainer>
-            <ActionIconButton icon={ImportIcon} style={'disabled'} buttonSize={ButtonSize.Small} onClick={onArcGisLoginClick}>
+            <ActionIconButton Icon={ImportIcon} style={'disabled'} size={ButtonSize.Small} onClick={onArcGisLoginClick}>
               Login to ArcGIS
             </ActionIconButton>
             <EsriStyledImage />
           </ActionIconButtonContainer>
         )}
         {showImport && (
           <ActionIconButtonContainer>
-            <ActionIconButton icon={ImportIcon} style={'active'} buttonSize={ButtonSize.Small} onClick={onArcGisImportClick}>
+            <ActionIconButton Icon={ImportIcon} style={'active'} size={ButtonSize.Small} onClick={onArcGisImportClick}>
               Import from ArcGIS
             </ActionIconButton>
             <EsriStyledImage />

diff --git a/src/components/layers-panel/map-options-panel.tsx b/src/components/layers-panel/map-options-panel.tsx
@@ -121,7 +121,7 @@ export const MapOptionPanel = ({ insertBaseMap }: MapOptionPanelProps) => {
         })}
       </MapList>
       <InsertButtons>
-        <ActionIconButton icon={PlusIcon} buttonSize={ButtonSize.Big} onClick={insertBaseMap}>
+        <ActionIconButton Icon={PlusIcon} size={ButtonSize.Big} onClick={insertBaseMap}>
           Insert Base Map
         </ActionIconButton>
       </InsertButtons>

diff --git a/src/components/logout-button/logout-button.spec.tsx b/src/components/logout-button/logout-button.spec.tsx
@@ -18,11 +18,12 @@ describe("Logout Button", () => {
     const { container } = callRender(renderWithTheme, { children: 'Test Button' });
     expect(container).toBeInTheDocument();
     const button = screen.getByText('Test Button');
-    const buttonHeight = getComputedStyle(button as Element).getPropertyValue(
-      "margin-left"
+    const buttonIcon = button.nextSibling as Element;
+    const buttonHeight = getComputedStyle(buttonIcon).getPropertyValue(
+      "height"
     );
-    expect(buttonHeight).toEqual('41px');
-    userEvent.click(button);
+    expect(buttonHeight).toEqual('17px');
+    userEvent.click(buttonIcon);
     expect(onClickMock).toHaveBeenCalled();
   });
 

diff --git a/src/components/logout-button/logout-button.tsx b/src/components/logout-button/logout-button.tsx
@@ -1,51 +1,61 @@
 import styled from "styled-components";
 import LogoutIcon from "../../../public/icons/logout.svg";
+import {
+  color_canvas_secondary_inverted,
+} from "../../constants/colors";
 
-type LogoutButtonProps = {
-  children?: React.ReactNode;
-  onClick?: () => void;
-};
-
-const LogoutImage = styled(LogoutIcon)`
+const ButtonContainer = styled.div`
   display: flex;
-  position: relative;
-  height: 14px;
+  justify-content: flex-start;
+  align-items: center;
+  padding-left: 10px;
+  transform: translate(0, -8px);
+
+  &:hover {
+    > :nth-child(2) > * {
+        stroke: ${({ theme }) => (theme.colors.logoutButtonIconColorHover)};
+      }
+  }
 `;
 
 const ButtonText = styled.div`
+  position: relative;
+  height: 17px;
+  margin-left: 40px;
+  margin-right: 8px;
   color: ${({ theme }) => (
-    theme.colors.actionIconButtonTextDisabledColor
+    theme.colors.logoutButtonTextColor
   )};
-  margin-left: 41px;
-  margin-right: 16px;
 `;
 
-const Button = styled.div`
-  display: flex;
+const IconContainer = styled.div`
+  position: relative;
+  width: 63px;
+  height: 17px;
   cursor: pointer;
-  justify-content: flex-start;
-  align-items: center;
+`;
 
-  &:hover {
-    > * {
-        background: ${({ theme }) => (
-    theme.colors.actionIconButtonDisabledBGHover
-  )};
-    }
-  }
+const StyledIcon = styled(LogoutIcon)`
+  position: absolute;
+  top: 1px;
+  stroke: ${color_canvas_secondary_inverted};
 `;
 
+type LogoutButtonProps = {
+  children?: React.ReactNode;
+  onClick?: () => void;
+};
+
 export const LogoutButton = ({
   children,
   onClick,
 }: LogoutButtonProps) => {
   return (
-    <Button onClick={onClick}>
+    <ButtonContainer>
       <ButtonText>{children}</ButtonText>
-      <LogoutImage />
-    </Button>
+      <IconContainer onClick={onClick}>
+        <StyledIcon />
+      </IconContainer>
+    </ButtonContainer>
   );
 };
-
-
-