feat(ui): looker, lookml - add banner to cross-link ingestion #6111
Conversation
| <CodeOutlined /> YAML | ||
| </StyledButton> | ||
| </ButtonsWrapper> | ||
| {(type === 'looker' || type === 'lookml') && ( |
There was a problem hiding this comment.
I would use constants defined in src/app/ingest/source/builder/constants.ts here
There was a problem hiding this comment.
Checked and fixed!!
| <Alert | ||
| type="warning" | ||
| banner | ||
| message={ | ||
| <> | ||
| <big> | ||
| <i> | ||
| <b>You must acknowledge this message to proceed!</b> | ||
| </i> | ||
| </big> | ||
| <br /> | ||
| <br /> | ||
| To get complete Looker metadata integration (including Looker views and lineage to the | ||
| underlying warehouse tables), you must <b>also</b> use the {link}. | ||
| </> | ||
| } | ||
| /> | ||
| )} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Created and fixed.
| <big> | ||
| <i> | ||
| <b>You must acknowledge this message to proceed!</b> | ||
| </i> | ||
| </big> | ||
| <br /> | ||
| <br /> |
There was a problem hiding this comment.
I don't think you need this part here anymore!
There was a problem hiding this comment.
Removed these lines and implemented them in a new component "LookerWarning".
| <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> |
There was a problem hiding this comment.
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;?
There was a problem hiding this comment.
Yes, this is another solution to implement this. I only used Row and Col at that time.
shirshanka
changed the title
| if (type === LOOKER) { | ||
| link = ( | ||
| <a href={LOOKER_DOC_LINK} target="_blank" rel="noopener noreferrer"> | ||
| DataHub looker module | ||
| </a> | ||
| ); | ||
| } else if (type === LOOK_ML) { | ||
| link = ( | ||
| <a href={LOOKML_DOC_LINK} target="_blank" rel="noopener noreferrer"> | ||
| DataHub lookml module | ||
| </a> | ||
| ); | ||
| } |
There was a problem hiding this comment.
can you swap these two links? If they're on the Looker form, we need to tell them to also use the LookML module. Then if they're on the LookML form, we need to tell them to use the Looker module.
There was a problem hiding this comment.
Checked and fixed it!!
| </ButtonsWrapper> | ||
| {(type === LOOKER || type === LOOK_ML) && <LookerWarning type={type} />} | ||
| <HeaderContainer> | ||
| <Title level={5}>{sourceConfigs?.displayName} Recipe</Title> |
There was a problem hiding this comment.
can you add style={{ marginBottom: 0 }} as an inline style to Title here? For some reason ant design always adds margin to the bottom of Typography.Title and you can't remove it using styled components so you have to do it inline.
There was a problem hiding this comment.
Checked and fixed!!
Checklist