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.

Sign up for GitHub

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

Jump to bottom

feat(ui): looker, lookml - add banner to cross-link ingestion #6111

Merged
merged 5 commits into from
Oct 11, 2022
Merged

feat(ui): looker, lookml - add banner to cross-link ingestion #6111

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9b80a5
Ingestion header modiification
Ankit-Keshari-Vituity Oct 4, 2022
20080a6
Worked on the PR comments
Ankit-Keshari-Vituity Oct 6, 2022
16ef58c
Removed the unwanted code
Ankit-Keshari-Vituity Oct 7, 2022
3c43099
Worked on the PR comments
Ankit-Keshari-Vituity Oct 9, 2022
2454d51
Merge branch 'ingestion-modification' of https://github.com/Ankit-Kes…
Ankit-Keshari-Vituity Oct 9, 2022
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
71 changes: 60 additions & 11 deletions datahub-web-react/src/app/ingest/source/builder/RecipeBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Button, message } from 'antd';
import { Button, Row, Col, message, Typography, Alert } from 'antd';
import React, { useState } from 'react';
import YAML from 'yamljs';
import { CodeOutlined, FormOutlined } from '@ant-design/icons';
Expand All @@ -8,6 +8,9 @@ import { YamlEditor } from './YamlEditor';
import RecipeForm from './RecipeForm/RecipeForm';
import { SourceBuilderState, SourceConfig } from './types';

const LOOKML_DOC_LINK = 'https://datahubproject.io/docs/generated/ingestion/sources/looker#module-lookml';
const LOOKER_DOC_LINK = 'https://datahubproject.io/docs/generated/ingestion/sources/looker#module-looker';

export const ControlsContainer = styled.div`
display: flex;
justify-content: space-between;
Expand All @@ -32,10 +35,15 @@ const StyledButton = styled(Button)<{ isSelected: boolean }>`
`}
`;

const Title = styled(Typography.Title)`
display: flex;
align-items: center;
justify-content: flex-start;
`;

const ButtonsWrapper = styled.div`
display: flex;
justify-content: flex-end;
margin-bottom: 10px;
`;

interface Props {
Expand All @@ -50,7 +58,7 @@ interface Props {

function RecipeBuilder(props: Props) {
const { state, isEditing, displayRecipe, sourceConfigs, setStagedRecipe, onClickNext, goToPrevious } = props;

const { type } = state;
const [isViewingForm, setIsViewingForm] = useState(true);

function switchViews(isFormView: boolean) {
Expand All @@ -65,16 +73,57 @@ function RecipeBuilder(props: Props) {
}
}

let link: React.ReactNode;
if (type === 'looker') {
link = (
<a href={LOOKER_DOC_LINK} target="_blank" rel="noopener noreferrer">
DataHub looker module
</a>
);
} else if (type === 'lookml') {
link = (
<a href={LOOKML_DOC_LINK} target="_blank" rel="noopener noreferrer">
DataHub lookml module
</a>
);
}

return (
<div>
<ButtonsWrapper>
<StyledButton type="text" isSelected={isViewingForm} onClick={() => switchViews(true)}>
<FormOutlined /> Form
</StyledButton>
<StyledButton type="text" isSelected={!isViewingForm} onClick={() => switchViews(false)}>
<CodeOutlined /> YAML
</StyledButton>
</ButtonsWrapper>
{(type === 'looker' || type === 'lookml') && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use constants defined in src/app/ingest/source/builder/constants.ts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked and fixed!!

<Alert
type="warning"
banner
message={
<>
<big>
<i>
<b>You must acknowledge this message to proceed!</b>
</i>
</big>
<br />
<br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need this part here anymore!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed these lines and implemented them in a new component "LookerWarning".

To get complete Looker metadata integration (including Looker views and lineage to the
underlying warehouse tables), you must <b>also</b> use the {link}.
</>
}
/>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can move this as well as the logic above regarding link into a new file and component called LookerWarning or something to keep this component a little cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created and fixed.

<Row style={{ marginBottom: '10px' }}>
<Col xl={8} lg={8} md={8} sm={24} xs={24}>
<Title level={5}>{sourceConfigs?.displayName} Recipe</Title>
</Col>
<Col xl={16} lg={16} md={16} sm={24} xs={24}>
<ButtonsWrapper>
<StyledButton type="text" isSelected={isViewingForm} onClick={() => switchViews(true)}>
<FormOutlined /> Form
</StyledButton>
<StyledButton type="text" isSelected={!isViewingForm} onClick={() => switchViews(false)}>
<CodeOutlined /> YAML
</StyledButton>
</ButtonsWrapper>
</Col>
</Row>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing wrong with using Row and Col here - just curious why you decided on that instead of just wrapping Title and ButtonsWrapper in a parent div with display: flex; justify-content: space-between; margin-bottom: 10px;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is another solution to implement this. I only used Row and Col at that time.

{isViewingForm && (
<RecipeForm
state={state}
Expand Down