-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Selector recommendations in Owner, Tag and Domain Modal #5197
Changes from all commits
ce46dc1
e5922eb
b67c7c1
dabfd53
87d9e76
dda06b7
1608caf
c406432
913c40e
ebe27e9
ea8eb79
e49e704
94b7cc2
a6fdde0
e3bd40c
b62b368
e1d6a54
2c1b200
607ff6b
be683d2
969095a
027283f
845e752
612d023
0077e8c
f7ffb22
74e223c
73d5275
fda9da0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,82 +1,89 @@ | ||
import { Button, Form, message, Modal, Select, Tag } from 'antd'; | ||
import React, { useRef, useState } from 'react'; | ||
import { Button, Form, message, Modal, Select, Tag } from 'antd'; | ||
import styled from 'styled-components'; | ||
import { Link } from 'react-router-dom'; | ||
|
||
import { useGetSearchResultsLazyQuery } from '../../../../../../../graphql/search.generated'; | ||
import { EntityType, SearchResult } from '../../../../../../../types.generated'; | ||
import { Entity, EntityType } from '../../../../../../../types.generated'; | ||
import { useSetDomainMutation } from '../../../../../../../graphql/mutations.generated'; | ||
import { useEntityRegistry } from '../../../../../../useEntityRegistry'; | ||
import { useEntityData } from '../../../../EntityContext'; | ||
import { useEnterKeyListener } from '../../../../../../shared/useEnterKeyListener'; | ||
import { useGetRecommendations } from '../../../../../../shared/recommendation'; | ||
import { DomainLabel } from '../../../../../../shared/DomainLabel'; | ||
|
||
type Props = { | ||
visible: boolean; | ||
onClose: () => void; | ||
onCloseModal: () => void; | ||
refetch?: () => Promise<any>; | ||
}; | ||
|
||
const SearchResultContainer = styled.div` | ||
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
padding: 12px; | ||
`; | ||
|
||
const SearchResultContent = styled.div` | ||
display: flex; | ||
justify-content: start; | ||
align-items: center; | ||
`; | ||
|
||
const SearchResultDisplayName = styled.div` | ||
margin-left: 12px; | ||
`; | ||
|
||
type SelectedDomain = { | ||
displayName: string; | ||
type: EntityType; | ||
urn: string; | ||
}; | ||
|
||
export const SetDomainModal = ({ visible, onClose, refetch }: Props) => { | ||
const StyleTag = styled(Tag)` | ||
padding: 0px 7px; | ||
margin-right: 3px; | ||
display: flex; | ||
justify-content: start; | ||
align-items: center; | ||
`; | ||
|
||
export const SetDomainModal = ({ onCloseModal, refetch }: Props) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove visible? |
||
const entityRegistry = useEntityRegistry(); | ||
const { urn } = useEntityData(); | ||
const [inputValue, setInputValue] = useState(''); | ||
const [selectedDomain, setSelectedDomain] = useState<SelectedDomain | undefined>(undefined); | ||
const [domainSearch, { data: domainSearchData }] = useGetSearchResultsLazyQuery(); | ||
const domainSearchResults = domainSearchData?.search?.searchResults || []; | ||
const domainSearchResults = | ||
domainSearchData?.search?.searchResults?.map((searchResult) => searchResult.entity) || []; | ||
const [setDomainMutation] = useSetDomainMutation(); | ||
|
||
const [recommendedData] = useGetRecommendations([EntityType.Domain]); | ||
const inputEl = useRef(null); | ||
|
||
const onOk = async () => { | ||
if (!selectedDomain) { | ||
return; | ||
} | ||
try { | ||
await setDomainMutation({ | ||
const onModalClose = () => { | ||
setInputValue(''); | ||
setSelectedDomain(undefined); | ||
onCloseModal(); | ||
}; | ||
|
||
const handleSearch = (text: string) => { | ||
if (text.length > 2) { | ||
domainSearch({ | ||
variables: { | ||
entityUrn: urn, | ||
domainUrn: selectedDomain.urn, | ||
input: { | ||
type: EntityType.Domain, | ||
query: text, | ||
start: 0, | ||
count: 5, | ||
}, | ||
}, | ||
}); | ||
message.success({ content: 'Updated Domain!', duration: 2 }); | ||
} catch (e: unknown) { | ||
message.destroy(); | ||
if (e instanceof Error) { | ||
message.error({ content: `Failed to set Domain: \n ${e.message || ''}`, duration: 3 }); | ||
} | ||
} | ||
setSelectedDomain(undefined); | ||
refetch?.(); | ||
onClose(); | ||
}; | ||
|
||
// Renders a search result in the select dropdown. | ||
const renderSearchResult = (entity: Entity) => { | ||
const displayName = entityRegistry.getDisplayName(entity.type, entity); | ||
return ( | ||
<Select.Option value={entity.urn} key={entity.urn}> | ||
<DomainLabel name={displayName} /> | ||
</Select.Option> | ||
); | ||
}; | ||
|
||
const domainResult = !inputValue || inputValue.length === 0 ? recommendedData : domainSearchResults; | ||
|
||
const domainSearchOptions = domainResult?.map((result) => { | ||
return renderSearchResult(result); | ||
}); | ||
|
||
const onSelectDomain = (newUrn: string) => { | ||
if (inputEl && inputEl.current) { | ||
(inputEl.current as any).blur(); | ||
} | ||
const filteredDomains = | ||
domainSearchResults?.filter((result) => result.entity.urn === newUrn).map((result) => result.entity) || []; | ||
const filteredDomains = domainResult?.filter((entity) => entity.urn === newUrn).map((entity) => entity) || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove this blur line? |
||
if (filteredDomains.length) { | ||
const domain = filteredDomains[0]; | ||
setSelectedDomain({ | ||
|
@@ -87,56 +94,67 @@ export const SetDomainModal = ({ visible, onClose, refetch }: Props) => { | |
} | ||
}; | ||
|
||
const handleSearch = (text: string) => { | ||
if (text.length > 2) { | ||
domainSearch({ | ||
const onDeselectDomain = () => { | ||
setInputValue(''); | ||
setSelectedDomain(undefined); | ||
}; | ||
|
||
const onOk = async () => { | ||
if (!selectedDomain) { | ||
return; | ||
} | ||
try { | ||
await setDomainMutation({ | ||
variables: { | ||
input: { | ||
type: EntityType.Domain, | ||
query: text, | ||
start: 0, | ||
count: 5, | ||
}, | ||
entityUrn: urn, | ||
domainUrn: selectedDomain.urn, | ||
}, | ||
}); | ||
message.success({ content: 'Updated Domain!', duration: 2 }); | ||
} catch (e: unknown) { | ||
message.destroy(); | ||
if (e instanceof Error) { | ||
message.error({ content: `Failed to set Domain: \n ${e.message || ''}`, duration: 3 }); | ||
} | ||
} | ||
setSelectedDomain(undefined); | ||
refetch?.(); | ||
onModalClose(); | ||
}; | ||
|
||
const selectValue = (selectedDomain && [selectedDomain?.displayName]) || undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a bunch of formatting changes. It makes this PR hard to use. |
||
|
||
// Handle the Enter press | ||
useEnterKeyListener({ | ||
querySelectorToExecuteClick: '#setDomainButton', | ||
}); | ||
|
||
const renderSearchResult = (result: SearchResult) => { | ||
const displayName = entityRegistry.getDisplayName(result.entity.type, result.entity); | ||
const tagRender = (props) => { | ||
// eslint-disable-next-line react/prop-types | ||
const { label, closable, onClose } = props; | ||
const onPreventMouseDown = (event) => { | ||
event.preventDefault(); | ||
event.stopPropagation(); | ||
}; | ||
return ( | ||
<SearchResultContainer> | ||
<SearchResultContent> | ||
<SearchResultDisplayName> | ||
<div>{displayName}</div> | ||
</SearchResultDisplayName> | ||
</SearchResultContent> | ||
<Link | ||
target="_blank" | ||
rel="noopener noreferrer" | ||
to={() => `/${entityRegistry.getPathName(result.entity.type)}/${result.entity.urn}`} | ||
> | ||
View | ||
</Link>{' '} | ||
</SearchResultContainer> | ||
<StyleTag onMouseDown={onPreventMouseDown} closable={closable} onClose={onClose}> | ||
{label} | ||
</StyleTag> | ||
); | ||
}; | ||
|
||
const selectValue = (selectedDomain && [selectedDomain?.displayName]) || []; | ||
function handleBlur() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that when we unfocus the text box is cleared? We probably don't want this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked and fixed. |
||
setInputValue(''); | ||
} | ||
|
||
return ( | ||
<Modal | ||
title="Set Domain" | ||
visible={visible} | ||
onCancel={onClose} | ||
visible | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove visible? |
||
onCancel={onModalClose} | ||
footer={ | ||
<> | ||
<Button onClick={onClose} type="text"> | ||
<Button onClick={onModalClose} type="text"> | ||
Cancel | ||
</Button> | ||
<Button id="setDomainButton" disabled={selectedDomain === undefined} onClick={onOk}> | ||
|
@@ -149,21 +167,26 @@ export const SetDomainModal = ({ visible, onClose, refetch }: Props) => { | |
<Form.Item> | ||
<Select | ||
autoFocus | ||
value={selectValue} | ||
defaultOpen | ||
filterOption={false} | ||
showSearch | ||
mode="multiple" | ||
ref={inputEl} | ||
defaultActiveFirstOption={false} | ||
placeholder="Search for Domains..." | ||
onSelect={(domainUrn: any) => onSelectDomain(domainUrn)} | ||
onDeselect={() => setSelectedDomain(undefined)} | ||
onSearch={handleSearch} | ||
filterOption={false} | ||
tagRender={(tagProps) => <Tag>{tagProps.value}</Tag>} | ||
onDeselect={onDeselectDomain} | ||
onSearch={(value: string) => { | ||
// eslint-disable-next-line react/prop-types | ||
handleSearch(value.trim()); | ||
// eslint-disable-next-line react/prop-types | ||
setInputValue(value.trim()); | ||
}} | ||
ref={inputEl} | ||
value={selectValue} | ||
tagRender={tagRender} | ||
onBlur={handleBlur} | ||
> | ||
{domainSearchResults.map((result) => { | ||
return ( | ||
<Select.Option value={result.entity.urn}>{renderSearchResult(result)}</Select.Option> | ||
); | ||
})} | ||
{domainSearchOptions} | ||
</Select> | ||
</Form.Item> | ||
</Form> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,13 +73,14 @@ export const SidebarDomainSection = () => { | |
</> | ||
)} | ||
</div> | ||
<SetDomainModal | ||
visible={showModal} | ||
refetch={refetch} | ||
onClose={() => { | ||
setShowModal(false); | ||
}} | ||
/> | ||
{showModal && ( | ||
<SetDomainModal | ||
refetch={refetch} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove visible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We worked on these together - there were unexpected side-effects from not unmounting the modal component and just passing in the The easiest solution where we don't need timeout hacks is to just unmount the whole modal component when it's not visible, so every time it's opened, it's opened fresh without any unexpected side-effects from stale state. |
||
onCloseModal={() => { | ||
setShowModal(false); | ||
}} | ||
/> | ||
)} | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the visible flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded to this and the few below on the last instance of this type of comment