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

feat: Add Table Notices #957

Merged
merged 8 commits into from
Apr 2, 2021
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
69 changes: 27 additions & 42 deletions amundsen_application/static/.betterer.results
Original file line number Diff line number Diff line change
Expand Up @@ -507,13 +507,13 @@ exports[`eslint`] = {
"js/components/Tags/index.tsx:3468508233": [
[38, 4, 21, "Must use destructuring props assignment", "4236634811"]
],
"js/config/config-default.ts:442852777": [
"js/config/config-default.ts:1637253929": [
[2, 0, 72, "\`../interfaces\` import should occur before import of \`./config-types\`", "1449508543"],
[220, 6, 21, "\'partitionKey\' is defined but never used.", "399589312"],
[221, 6, 23, "\'partitionValue\' is defined but never used.", "793372348"]
[222, 6, 21, "\'partitionKey\' is defined but never used.", "399589312"],
[223, 6, 23, "\'partitionValue\' is defined but never used.", "793372348"]
],
"js/config/config-utils.ts:108982451": [
[7, 0, 45, "\`../interfaces\` import should occur before import of \`./config-types\`", "3885176344"]
"js/config/config-utils.ts:157517129": [
[11, 0, 45, "\`../interfaces\` import should occur before import of \`./config-types\`", "3885176344"]
],
"js/ducks/announcements/api/v0.ts:908063915": [
[30, 13, 64, "Expected the Promise rejection reason to be an Error.", "3665078104"]
Expand Down Expand Up @@ -769,39 +769,25 @@ exports[`eslint`] = {
[89, 25, 5, "\'props\' is assigned a value but never used.", "187023499"],
[100, 27, 5, "\'props\' is assigned a value but never used.", "187023499"]
],
"js/pages/DashboardPage/index.spec.tsx:3246337270": [
"js/pages/DashboardPage/index.spec.tsx:3159805396": [
[22, 0, 63, "\`../../fixtures/mockRouter\` import should occur before import of \`./ChartList\`", "1389010998"],
[24, 0, 41, "\`./constants\` import should occur before import of \`.\`", "1965203596"],
[59, 56, 10, "Prop spreading is forbidden", "480399587"],
[139, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[168, 16, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[184, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[202, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[242, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"]
],
"js/pages/DashboardPage/index.tsx:277488038": [
[82, 20, 16, "Must use destructuring props assignment", "1899951550"],
[88, 47, 19, "Must use destructuring props assignment", "1779396464"],
[89, 20, 16, "Must use destructuring props assignment", "1899951550"],
[91, 4, 23, "Must use destructuring props assignment", "3651858367"],
[96, 20, 16, "Must use destructuring props assignment", "1899951550"],
[98, 8, 14, "Must use destructuring state assignment", "1770565882"],
[99, 49, 19, "Must use destructuring props assignment", "1779396464"],
[100, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[101, 6, 23, "Must use destructuring props assignment", "3651858367"],
[106, 23, 20, "\'searchDashboardGroup\' is already declared in the upper scope.", "2919356688"],
[127, 20, 20, "Must use destructuring props assignment", "3424894889"],
[133, 24, 20, "Must use destructuring props assignment", "3424894889"],
[136, 8, 20, "Must use destructuring props assignment", "3424894889"],
[138, 36, 20, "Must use destructuring props assignment", "3424894889"],
[140, 26, 20, "Must use destructuring props assignment", "3424894889"],
[147, 19, 20, "Must use destructuring props assignment", "3424894889"],
[148, 19, 20, "Must use destructuring props assignment", "3424894889"],
[152, 25, 20, "Must use destructuring props assignment", "3424894889"],
[168, 8, 21, "Must use destructuring props assignment", "3005585300"],
[172, 10, 7, "A form label must be associated with a control.", "2729454337"],
[295, 28, 20, "Must use destructuring props assignment", "3424894889"],
[336, 31, 14, "Must use destructuring state assignment", "1770565882"]
[60, 56, 10, "Prop spreading is forbidden", "480399587"],
[140, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[169, 16, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[185, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[203, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"],
[243, 14, 7, "\'wrapper\' is already declared in the upper scope.", "990908086"]
],
"js/pages/DashboardPage/index.tsx:3355322503": [
[88, 20, 16, "Must use destructuring props assignment", "1899951550"],
[94, 22, 12, "\'getDashboard\' is already declared in the upper scope.", "2023796375"],
[103, 22, 12, "\'getDashboard\' is already declared in the upper scope.", "2023796375"],
[106, 8, 14, "Must use destructuring state assignment", "1770565882"],
[108, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[114, 23, 20, "\'searchDashboardGroup\' is already declared in the upper scope.", "2919356688"],
[182, 10, 7, "A form label must be associated with a control.", "2729454337"],
[352, 31, 14, "Must use destructuring state assignment", "1770565882"]
],
"js/pages/HomePage/index.spec.tsx:4057001982": [
[26, 48, 10, "Prop spreading is forbidden", "480399587"],
Expand Down Expand Up @@ -1049,12 +1035,11 @@ exports[`eslint`] = {
"js/pages/TableDetailPage/index.spec.tsx:163729336": [
[57, 6, 25, "Use object destructuring.", "1230260048"]
],
"js/pages/TableDetailPage/index.tsx:1290852100": [
[126, 2, 20, "key should be placed after componentDidUpdate", "3916788587"],
[136, 22, 12, "\'getTableData\' is already declared in the upper scope.", "3938384029"],
[145, 22, 12, "\'getTableData\' is already declared in the upper scope.", "3938384029"],
[183, 4, 23, "Must use destructuring props assignment", "2314094642"],
[220, 6, 28, "\'openRequestDescriptionDialog\' is already declared in the upper scope.", "4102713454"]
"js/pages/TableDetailPage/index.tsx:2815038016": [
[129, 2, 20, "key should be placed after componentDidUpdate", "3916788587"],
[139, 22, 12, "\'getTableData\' is already declared in the upper scope.", "3938384029"],
[148, 22, 12, "\'getTableData\' is already declared in the upper scope.", "3938384029"],
[224, 6, 28, "\'openRequestDescriptionDialog\' is already declared in the upper scope.", "4102713454"]
],
"js/utils/textUtils.ts:2545492889": [
[19, 6, 46, "Unexpected lexical declaration in case block.", "156477898"]
Expand Down
36 changes: 36 additions & 0 deletions amundsen_application/static/js/components/Alert/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import * as React from 'react';
import { mount } from 'enzyme';

import { NoticeSeverity } from 'config/config-types';

import Alert, { AlertProps } from '.';

const setup = (propOverrides?: Partial<AlertProps>) => {
Expand Down Expand Up @@ -96,6 +98,40 @@ describe('Alert', () => {
expect(actual).toEqual(expected);
});
});

describe('when passing a severity', () => {
it('should render the alert icon by default', () => {
const { wrapper } = setup();
const expected = 1;
const actual = wrapper.find('.alert-triangle-svg-icon').length;

expect(actual).toEqual(expected);
});

it('should render the info icon when info severity', () => {
const { wrapper } = setup({ severity: NoticeSeverity.INFO });
const expected = 1;
const actual = wrapper.find('.info-svg-icon').length;

expect(actual).toEqual(expected);
});

it('should render the alert icon when alert severity', () => {
const { wrapper } = setup({ severity: NoticeSeverity.ALERT });
const expected = 1;
const actual = wrapper.find('.alert-triangle-svg-icon').length;

expect(actual).toEqual(expected);
});

it('should render the alert icon when warning severity', () => {
const { wrapper } = setup({ severity: NoticeSeverity.WARNING });
const expected = 1;
const actual = wrapper.find('.alert-triangle-svg-icon').length;

expect(actual).toEqual(expected);
});
});
});

describe('lifetime', () => {
Expand Down
37 changes: 33 additions & 4 deletions amundsen_application/static/js/components/Alert/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,23 @@
// SPDX-License-Identifier: Apache-2.0

import * as React from 'react';
import SanitizedHTML from 'react-sanitized-html';

import { IconSizes } from 'interfaces';
import { AlertIcon } from '../SVGIcons';
import { NoticeSeverity } from 'config/config-types';
import { AlertIcon, InformationIcon } from '../SVGIcons';

import './styles.scss';

const STROKE_COLOR = '#b8072c'; // $red70
const SEVERITY_TO_COLOR_MAP = {
[NoticeSeverity.INFO]: '#3a97d3', // cyan50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have these colors on the _colors css file as variables?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the 'cyan50', but we cannot bring the scss variables into the JS files :(

[NoticeSeverity.WARNING]: '#ffb146', // $amber50
[NoticeSeverity.ALERT]: '#b8072c', // $red70
};

export interface AlertProps {
message: string | React.ReactNode;
severity?: NoticeSeverity;
actionLink?: React.ReactNode;
actionText?: string;
actionHref?: string;
Expand All @@ -20,6 +27,7 @@ export interface AlertProps {

const Alert: React.FC<AlertProps> = ({
message,
severity = NoticeSeverity.WARNING,
danwom marked this conversation as resolved.
Show resolved Hide resolved
onAction,
actionText,
actionHref,
Expand Down Expand Up @@ -51,10 +59,31 @@ const Alert: React.FC<AlertProps> = ({
action = <span className="alert-action">{actionLink}</span>;
}

let iconComponent: React.ReactNode = null;
if (severity === NoticeSeverity.INFO) {
iconComponent = (
<InformationIcon
fill={SEVERITY_TO_COLOR_MAP[severity]}
size={IconSizes.REGULAR}
/>
);
} else {
iconComponent = (
<AlertIcon
stroke={SEVERITY_TO_COLOR_MAP[severity]}
size={IconSizes.SMALL}
/>
);
}

// If we receive a string, we want to sanitize any html inside
const formattedMessage =
typeof message === 'string' ? <SanitizedHTML html={message} /> : message;

return (
<div className="alert">
<AlertIcon stroke={STROKE_COLOR} size={IconSizes.SMALL} />
<p className="alert-message">{message}</p>
{iconComponent}
<p className="alert-message">{formattedMessage}</p>
{action}
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ $alert-message-line-height: 24px;
display: inline;
}

.alert-triangle-svg-icon {
.alert-triangle-svg-icon,
.info-svg-icon {
flex-shrink: 0;
align-self: center;
margin-right: $spacer-1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ export const InformationIcon: React.FC<IconProps> = ({
const id = `info_icon_${uuidv4()}`;

return (
<svg width={size} height={size} viewBox="0 0 24 24">
<svg
width={size}
height={size}
viewBox="0 0 24 24"
className="info-svg-icon"
>
<title>Info</title>
<defs>
<path
Expand Down
2 changes: 2 additions & 0 deletions amundsen_application/static/js/config/config-default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const configDefault: AppConfig = {
type: FilterType.INPUT_SELECT,
},
],
notices: {},
},
[ResourceType.table]: {
displayName: 'Datasets',
Expand Down Expand Up @@ -193,6 +194,7 @@ const configDefault: AppConfig = {
iconPath: '/static/images/github.png',
},
},
notices: {},
},
[ResourceType.user]: {
displayName: 'People',
Expand Down
16 changes: 16 additions & 0 deletions amundsen_application/static/js/config/config-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,28 @@ type SortCriteriaConfig = {
[key: string]: SortCriteria;
};

export enum NoticeSeverity {
INFO = 'info',
WARNING = 'warning',
ALERT = 'alert',
}
export interface NoticeType {
severity: NoticeSeverity;
messageHtml: string;
}
/**
* Stats configuration options
*/
type StatsConfig = {
uniqueValueTypeName: string;
};

/**
* A list of notices where the key is the 'schema.name' of the table and the value
* a Notice
*/
type NoticesConfigType = Record<string, NoticeType>;

/**
* Base interface for all possible ResourceConfig objects
*
Expand All @@ -157,6 +172,7 @@ interface BaseResourceConfig {
displayName: string;
filterCategories?: FilterConfig;
supportedSources?: SourcesConfig;
notices?: NoticesConfigType;
}

interface TableResourceConfig extends BaseResourceConfig {
Expand Down
26 changes: 23 additions & 3 deletions amundsen_application/static/js/config/config-utils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import AppConfig from 'config/config';
import { BadgeStyle, BadgeStyleConfig } from 'config/config-types';
import { TableMetadata } from 'interfaces/TableMetadata';
import { convertText, CaseType } from 'utils/textUtils';

import { AnalyticsConfig, FilterConfig, LinkConfig } from './config-types';

import { TableMetadata } from 'interfaces/TableMetadata';
import {
AnalyticsConfig,
FilterConfig,
LinkConfig,
NoticeType,
} from './config-types';
import { ResourceType } from '../interfaces';

export const DEFAULT_DATABASE_ICON_CLASS = 'icon-database icon-color';
Expand Down Expand Up @@ -60,6 +64,22 @@ export function getSourceIconClass(
return config.supportedSources[sourceId].iconClass;
}

/**
* Returns notices for the given resource name if present
*/
export function getResourceNotices(
resourceType: ResourceType,
resourceName: string
): NoticeType | false {
const { notices } = AppConfig.resourceConfig[resourceType];

if (notices && notices[resourceName]) {
return notices[resourceName];
}

return false;
}

/**
* Returns the displayName for the given resourceType
*/
Expand Down
Loading