Added suggestions from code review

elastic · Jul 12, 2021 · bc3df22 · bc3df22
1 parent 7668cdb
commit bc3df22
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 56 deletions.
diff --git a/.../public/management/edit_space/customize_space/__snapshots__/customize_space.test.tsx.snap b/.../public/management/edit_space/customize_space/__snapshots__/customize_space.test.tsx.snap
diff --git a/.../management/edit_space/customize_space/__snapshots__/customize_space_avatar.test.tsx.snap b/.../management/edit_space/customize_space/__snapshots__/customize_space_avatar.test.tsx.snap
diff --git a/x-pack/plugins/spaces/public/management/edit_space/customize_space/customize_space.tsx b/x-pack/plugins/spaces/public/management/edit_space/customize_space/customize_space.tsx
@@ -104,6 +104,9 @@ export class CustomizeSpace extends Component<Props, State> {
                 defaultMessage: 'Description',
               }
             )}
+            labelAppend={i18n.translate('xpack.spaces.management.manageSpacePage.optionalLabel', {
+              defaultMessage: 'Optional',
+            })}
             helpText={i18n.translate(
               'xpack.spaces.management.manageSpacePage.spaceDescriptionHelpText',
               {
@@ -170,7 +173,7 @@ export class CustomizeSpace extends Component<Props, State> {
                   defaultMessage: 'Choose how your space avatar appears across Kibana.',
                 })}
               </p>
-              {space.avatarType === 'image' && space.imageUrl ? (
+              {space.avatarType === 'image' ? (
                 <Suspense fallback={<EuiLoadingSpinner />}>
                   <LazySpaceAvatar
                     space={{
@@ -181,27 +184,17 @@ export class CustomizeSpace extends Component<Props, State> {
                     size="xl"
                   />
                 </Suspense>
-              ) : space.avatarType !== 'image' && (space.name || space.initials) ? (
+              ) : (
                 <Suspense fallback={<EuiLoadingSpinner />}>
                   <LazySpaceAvatar
                     space={{
+                      name: '?',
                       ...space,
                       imageUrl: undefined,
                     }}
                     size="xl"
                   />
                 </Suspense>
-              ) : (
-                <EuiAvatar
-                  type="space"
-                  name="?"
-                  size="xl"
-                  color={null}
-                  style={{
-                    background: euiThemeVars.euiColorLightShade,
-                    color: euiThemeVars.euiTextSubduedColor,
-                  }}
-                />
               )}
             </>
           }

diff --git a/...ck/plugins/spaces/public/management/edit_space/customize_space/customize_space_avatar.tsx b/...ck/plugins/spaces/public/management/edit_space/customize_space/customize_space_avatar.tsx
@@ -127,6 +127,7 @@ export class CustomizeSpaceAvatar extends Component<Props> {
                 avatarType: avatarType as FormValues['avatarType'],
               })
             }
+            buttonSize="m"
           />
         </EuiFormRow>
         {space.avatarType !== 'image' ? (

diff --git a/x-pack/plugins/spaces/public/management/edit_space/manage_space_page.tsx b/x-pack/plugins/spaces/public/management/edit_space/manage_space_page.tsx
@@ -11,6 +11,7 @@ import {
   EuiFlexGroup,
   EuiFlexItem,
   EuiPageContent,
+  EuiPageContentBody,
   EuiPageHeader,
   EuiSpacer,
   hexToHsv,
@@ -128,16 +129,12 @@ export class ManageSpacePage extends Component<Props, State> {
     }
 
     return (
-      <>
-        <EuiPageHeader
-          bottomBorder
-          pageTitle={this.getTitle()}
-          description={getSpacesFeatureDescription()}
-        />
+      <EuiPageContentBody restrictWidth>
+        <EuiPageHeader pageTitle={this.getTitle()} description={getSpacesFeatureDescription()} />
         <EuiSpacer size="l" />
 
         {this.getForm()}
-      </>
+      </EuiPageContentBody>
     );
   }
 

diff --git a/x-pack/plugins/spaces/public/management/lib/validate_space.test.ts b/x-pack/plugins/spaces/public/management/lib/validate_space.test.ts
@@ -172,15 +172,6 @@ describe('validateAvatarColor', () => {
 
     expect(validator.validateAvatarColor(space)).toHaveProperty('isInvalid', true);
   });
-
-  it('it does not validate image avatars', () => {
-    const space = {
-      avatarType: 'image' as 'image',
-      color: '',
-    };
-
-    expect(validator.validateAvatarColor(space)).toHaveProperty('isInvalid', false);
-  });
 });
 
 describe('validateAvatarImage', () => {

diff --git a/x-pack/plugins/spaces/public/management/lib/validate_space.ts b/x-pack/plugins/spaces/public/management/lib/validate_space.ts
@@ -134,22 +134,20 @@ export class SpaceValidator {
       return valid();
     }
 
-    if (space.avatarType !== 'image') {
-      if (!space.color) {
-        return invalid(
-          i18n.translate('xpack.spaces.management.validateSpace.requiredColorErrorMessage', {
-            defaultMessage: 'Select a color.',
-          })
-        );
-      }
+    if (!space.color) {
+      return invalid(
+        i18n.translate('xpack.spaces.management.validateSpace.requiredColorErrorMessage', {
+          defaultMessage: 'Select a background color.',
+        })
+      );
+    }
 
-      if (!isValidHex(space.color)) {
-        return invalid(
-          i18n.translate('xpack.spaces.management.validateSpace.invalidColorErrorMessage', {
-            defaultMessage: 'Enter a valid HEX color code.',
-          })
-        );
-      }
+    if (!isValidHex(space.color)) {
+      return invalid(
+        i18n.translate('xpack.spaces.management.validateSpace.invalidColorErrorMessage', {
+          defaultMessage: 'Enter a valid HEX color code.',
+        })
+      );
     }
 
     return valid();