[Branding] handle comments from PR

Handling the helper function rename and grammar issues. To avoid risk, we will not remove the duplicate code for 1.2 and everything related to those comments (ie function renames). That will be handled in 1.3. Here is the issue tracking it: opensearch-project#895 Signed-off-by: Kawika Avilla <[email protected]>
kavilla · Oct 30, 2021 · 48e1c42 · 48e1c42
1 parent 11f90a9
commit 48e1c42
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 35 deletions.
diff --git a/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.tsx b/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.tsx
@@ -35,13 +35,13 @@ import '../header_logo.scss';
 import { ChromeBranding } from '../../../chrome_service';
 
 /**
- * Use branding configurations to render the header logo on the nab bar.
+ * Use branding configurations to render the header logo on the nav bar.
  *
  * @param {ChromeBranding} - branding object consist of logo, mark and title
- * @returns A image component which is going to be rendered on the main page header bar.
+ * @returns Custom branding logo component which is going to be rendered on the main page header bar.
  *          If logo default is valid, the full logo by logo default config will be rendered;
  *          if not, the logo icon by mark default config will be rendered; if both are not found,
- *          the default opensearch logo will be rendered.
+ *          the default OpenSearch Dashboards logo will be rendered.
  */
 export const CustomLogo = ({ ...branding }: ChromeBranding) => {
   const darkMode = branding.darkMode;
@@ -78,10 +78,7 @@ export const CustomLogo = ({ ...branding }: ChromeBranding) => {
    * @returns a valid custom header logo URL, or undefined
    */
   const customHeaderLogo = () => {
-    if (darkMode) {
-      return customHeaderLogoDarkMode();
-    }
-    return customHeaderLogoDefaultMode();
+    return darkMode ? customHeaderLogoDarkMode() : customHeaderLogoDefaultMode();
   };
 
   return customHeaderLogo() ? (

diff --git a/src/core/server/rendering/__mocks__/rendering_service.ts b/src/core/server/rendering/__mocks__/rendering_service.ts
@@ -41,13 +41,13 @@ export const setupMock: jest.Mocked<InternalRenderingServiceSetup> = {
 };
 export const mockSetup = jest.fn().mockResolvedValue(setupMock);
 export const mockStop = jest.fn();
-export const mockCheckUrlValid = jest.fn();
-export const mockCheckTitleValid = jest.fn();
+export const mockIsUrlValid = jest.fn();
+export const mockIsTitleValid = jest.fn();
 export const mockRenderingService: jest.Mocked<IRenderingService> = {
   setup: mockSetup,
   stop: mockStop,
-  checkUrlValid: mockCheckUrlValid,
-  checkTitleValid: mockCheckTitleValid,
+  isUrlValid: mockIsUrlValid,
+  isTitleValid: mockIsTitleValid,
 };
 export const RenderingService = jest.fn<IRenderingService, [typeof mockRenderingServiceParams]>(
   () => mockRenderingService

diff --git a/src/core/server/rendering/rendering_service.test.ts b/src/core/server/rendering/rendering_service.test.ts
@@ -137,50 +137,47 @@ describe('RenderingService', () => {
     });
   });
 
-  describe('checkUrlValid()', () => {
+  describe('isUrlValid()', () => {
     it('checks valid SVG URL', async () => {
-      const result = await service.checkUrlValid(
+      const result = await service.isUrlValid(
         'https://opensearch.org/assets/brand/SVG/Mark/opensearch_mark_default.svg',
         'config'
       );
       expect(result).toEqual(true);
     });
 
     it('checks valid PNG URL', async () => {
-      const result = await service.checkUrlValid(
+      const result = await service.isUrlValid(
         'https://opensearch.org/assets/brand/PNG/Mark/opensearch_mark_default.png',
         'config'
       );
       expect(result).toEqual(true);
     });
 
     it('checks invalid URL that does not contain svg, png or gif', async () => {
-      const result = await service.checkUrlValid('https://validUrl', 'config');
+      const result = await service.isUrlValid('https://validUrl', 'config');
       expect(result).toEqual(false);
     });
 
     it('checks invalid URL', async () => {
-      const result = await service.checkUrlValid('http://notfound.svg', 'config');
+      const result = await service.isUrlValid('http://notfound.svg', 'config');
       expect(result).toEqual(false);
     });
   });
 
-  describe('checkTitleValid()', () => {
+  describe('isTitleValid()', () => {
     it('checks valid title', () => {
-      const result = service.checkTitleValid('OpenSearch Dashboards', 'config');
+      const result = service.isTitleValid('OpenSearch Dashboards', 'config');
       expect(result).toEqual(true);
     });
 
     it('checks invalid title with empty string', () => {
-      const result = service.checkTitleValid('', 'config');
+      const result = service.isTitleValid('', 'config');
       expect(result).toEqual(false);
     });
 
     it('checks invalid title with length > 36 character', () => {
-      const result = service.checkTitleValid(
-        'OpenSearch Dashboardssssssssssssssssssssss',
-        'config'
-      );
+      const result = service.isTitleValid('OpenSearch Dashboardssssssssssssssssssssss', 'config');
       expect(result).toEqual(false);
     });
   });

diff --git a/src/core/server/rendering/rendering_service.tsx b/src/core/server/rendering/rendering_service.tsx
@@ -249,8 +249,8 @@ export class RenderingService {
 
   /**
    * Assign boolean values for branding related configurations to indicate if
-   * user inputs valid or invalid URLs by calling checkUrlValid() function. Also
-   * check if title is valid by calling checkTitleValid() function.
+   * user inputs valid or invalid URLs by calling isUrlValid() function. Also
+   * check if title is valid by calling isTitleValid() function.
    *
    * @param {boolean} darkMode
    * @param {Readonly<OpenSearchDashboardsConfigType>} opensearchDashboardsConfig
@@ -261,30 +261,30 @@ export class RenderingService {
     opensearchDashboardsConfig: Readonly<OpenSearchDashboardsConfigType>
   ): Promise<BrandingValidation> => {
     const branding = opensearchDashboardsConfig.branding;
-    const isLogoDefaultValid = await this.checkUrlValid(branding.logo.defaultUrl, 'logo default');
+    const isLogoDefaultValid = await this.isUrlValid(branding.logo.defaultUrl, 'logo default');
 
     const isLogoDarkmodeValid = darkMode
-      ? await this.checkUrlValid(branding.logo.darkModeUrl, 'logo darkMode')
+      ? await this.isUrlValid(branding.logo.darkModeUrl, 'logo darkMode')
       : false;
 
-    const isMarkDefaultValid = await this.checkUrlValid(branding.mark.defaultUrl, 'mark default');
+    const isMarkDefaultValid = await this.isUrlValid(branding.mark.defaultUrl, 'mark default');
 
     const isMarkDarkmodeValid = darkMode
-      ? await this.checkUrlValid(branding.mark.darkModeUrl, 'mark darkMode')
+      ? await this.isUrlValid(branding.mark.darkModeUrl, 'mark darkMode')
       : false;
 
-    const isLoadingLogoDefaultValid = await this.checkUrlValid(
+    const isLoadingLogoDefaultValid = await this.isUrlValid(
       branding.loadingLogo.defaultUrl,
       'loadingLogo default'
     );
 
     const isLoadingLogoDarkmodeValid = darkMode
-      ? await this.checkUrlValid(branding.loadingLogo.darkModeUrl, 'loadingLogo darkMode')
+      ? await this.isUrlValid(branding.loadingLogo.darkModeUrl, 'loadingLogo darkMode')
       : false;
 
-    const isFaviconValid = await this.checkUrlValid(branding.faviconUrl, 'favicon');
+    const isFaviconValid = await this.isUrlValid(branding.faviconUrl, 'favicon');
 
-    const isTitleValid = this.checkTitleValid(branding.applicationTitle, 'applicationTitle');
+    const isTitleValid = this.isTitleValid(branding.applicationTitle, 'applicationTitle');
 
     const brandingValidation: BrandingValidation = {
       isLogoDefaultValid,
@@ -314,7 +314,7 @@ export class RenderingService {
    * @param {string} configName
    * @returns {boolean} indicate if the URL is valid/invalid
    */
-  public checkUrlValid = async (url: string, configName?: string): Promise<boolean> => {
+  public isUrlValid = async (url: string, configName?: string): Promise<boolean> => {
     if (url.match(/\.(png|svg|gif|PNG|SVG|GIF)$/) === null) {
       this.logger.get('branding').info(configName + ' config is not found or invalid.');
       return false;
@@ -337,7 +337,7 @@ export class RenderingService {
    * @param {string} configName
    * @returns {boolean} indicate if user input title is valid/invalid
    */
-  public checkTitleValid = (title: string, configName?: string): boolean => {
+  public isTitleValid = (title: string, configName?: string): boolean => {
     if (!title || title.length > 36) {
       this.logger
         .get('branding')