Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Branding] handle comments from PR #897

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() ? (
Expand Down
8 changes: 4 additions & 4 deletions src/core/server/rendering/__mocks__/rendering_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 9 additions & 12 deletions src/core/server/rendering/rendering_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Expand Down
24 changes: 12 additions & 12 deletions src/core/server/rendering/rendering_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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')
Expand Down