From 5edf03062d40a92385070f53904b2344c0ffd351 Mon Sep 17 00:00:00 2001 From: Kawika Avilla Date: Sat, 30 Oct 2021 00:25:40 +0000 Subject: [PATCH] [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: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/895 Signed-off-by: Kawika Avilla --- .../opensearch_dashboards_custom_logo.tsx | 11 ++++----- .../chrome/ui/header/collapsible_nav.tsx | 5 +--- .../rendering/__mocks__/rendering_service.ts | 8 +++---- .../rendering/rendering_service.test.ts | 21 +++++++--------- .../server/rendering/rendering_service.tsx | 24 +++++++++---------- src/core/server/rendering/views/template.tsx | 5 +--- .../solutions_section/solution_title.tsx | 5 +--- .../overview_page_header.tsx | 5 +--- 8 files changed, 33 insertions(+), 51 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 index 34c69582ca7d..aaa12a79a077 100644 --- 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/public/chrome/ui/header/collapsible_nav.tsx b/src/core/public/chrome/ui/header/collapsible_nav.tsx index 5af6f157914d..0ed510933257 100644 --- a/src/core/public/chrome/ui/header/collapsible_nav.tsx +++ b/src/core/public/chrome/ui/header/collapsible_nav.tsx @@ -173,10 +173,7 @@ export function CollapsibleNav({ * @returns a valid logo URL */ const customSideMenuLogo = () => { - if (darkMode) { - return customSideMenuLogoDarkMode(); - } - return customSideMenuLogoDefaultMode(); + return darkMode ? customSideMenuLogoDarkMode() : customSideMenuLogoDefaultMode(); }; return ( diff --git a/src/core/server/rendering/__mocks__/rendering_service.ts b/src/core/server/rendering/__mocks__/rendering_service.ts index d4e0ede01828..98576eea07cd 100644 --- 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 = { }; 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 = { setup: mockSetup, stop: mockStop, - checkUrlValid: mockCheckUrlValid, - checkTitleValid: mockCheckTitleValid, + isUrlValid: mockIsUrlValid, + isTitleValid: mockIsTitleValid, }; export const RenderingService = jest.fn( () => mockRenderingService diff --git a/src/core/server/rendering/rendering_service.test.ts b/src/core/server/rendering/rendering_service.test.ts index ac726838d431..61a7f0ce3dea 100644 --- a/src/core/server/rendering/rendering_service.test.ts +++ b/src/core/server/rendering/rendering_service.test.ts @@ -137,9 +137,9 @@ 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' ); @@ -147,7 +147,7 @@ describe('RenderingService', () => { }); 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' ); @@ -155,32 +155,29 @@ describe('RenderingService', () => { }); 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 index 2d32c03f87ef..efaab2b94947 100644 --- 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} opensearchDashboardsConfig @@ -261,30 +261,30 @@ export class RenderingService { opensearchDashboardsConfig: Readonly ): Promise => { 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 => { + public isUrlValid = async (url: string, configName?: string): Promise => { 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') diff --git a/src/core/server/rendering/views/template.tsx b/src/core/server/rendering/views/template.tsx index 1daff8bc6cea..51da4dc59cdc 100644 --- a/src/core/server/rendering/views/template.tsx +++ b/src/core/server/rendering/views/template.tsx @@ -133,10 +133,7 @@ export const Template: FunctionComponent = ({ * @returns a valid custom loading logo URL, or undefined */ const customLoadingLogo = () => { - if (darkMode) { - return customLoadingLogoDarkMode(); - } - return customLoadingLogoDefaultMode(); + return darkMode ? customLoadingLogoDarkMode() : customLoadingLogoDefaultMode(); }; /** diff --git a/src/plugins/home/public/application/components/solutions_section/solution_title.tsx b/src/plugins/home/public/application/components/solutions_section/solution_title.tsx index aed7ebde8fab..35af9b55bb4d 100644 --- a/src/plugins/home/public/application/components/solutions_section/solution_title.tsx +++ b/src/plugins/home/public/application/components/solutions_section/solution_title.tsx @@ -91,10 +91,7 @@ const customHomeLogoDarkMode = (branding: HomePluginBranding) => { * @returns {string|undefined} a valid custom loading logo URL, or undefined */ const customHomeLogo = (branding: HomePluginBranding) => { - if (branding.darkMode) { - return customHomeLogoDarkMode(branding); - } - return customHomeLogoDefaultMode(branding); + return branding.darkMode ? customHomeLogoDarkMode(branding) : customHomeLogoDefaultMode(branding); }; /** diff --git a/src/plugins/opensearch_dashboards_react/public/overview_page/overview_page_header/overview_page_header.tsx b/src/plugins/opensearch_dashboards_react/public/overview_page/overview_page_header/overview_page_header.tsx index 10095056e7a8..ea21ddc92300 100644 --- a/src/plugins/opensearch_dashboards_react/public/overview_page/overview_page_header/overview_page_header.tsx +++ b/src/plugins/opensearch_dashboards_react/public/overview_page/overview_page_header/overview_page_header.tsx @@ -115,10 +115,7 @@ export const OverviewPageHeader: FC = ({ * @returns a valid custom loading logo URL, or undefined */ const customOverviewLogo = () => { - if (darkMode) { - return customOverviewLogoDarkMode(); - } - return customOverviewLogoDefaultMode(); + return darkMode ? customOverviewLogoDarkMode() : customOverviewLogoDefaultMode(); }; /**