-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
🪟 Add catalog changes modal on schema refresh #14074
Changes from all commits
c6fd501
371b2f1
48bb1c0
6ae438f
837e649
a30247b
93a4c21
9fda9f8
a9b9560
05ab7f5
fbd4760
b215e5e
c9daed9
f94c2af
9fd2e03
a5d47f8
af61e73
05dfe0a
0cd64ab
631faef
81b8d6c
584d07c
90498e9
5043f63
4cad388
5c95e0e
c1ce5ea
7ef0651
0f0bbdb
4a4800a
1d0ca6d
1014d26
37f713b
b769506
e5a9076
fdc8ac5
4d79876
b62b67e
a03f452
72e7a79
f4870f1
50037d7
db022de
d611e4b
64f61db
fa18475
7c23669
bb9e18d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
export const ModificationIcon: React.FC = () => { | ||
return ( | ||
<svg width="20" height="20" viewBox="0 0 20 20" fill="none"> | ||
<path | ||
fillRule="evenodd" | ||
clipRule="evenodd" | ||
d="M10.8332 9.16663L14.1665 6.66663L10.8332 4.16663V5.83329H4.1665V7.49996H10.8332V9.16663Z" | ||
fill="#CBC8FF" | ||
/> | ||
<path | ||
fillRule="evenodd" | ||
clipRule="evenodd" | ||
d="M9.16683 15.8334L5.8335 13.3334L9.16683 10.8334V12.5H15.8335V14.1667H9.16683V15.8334Z" | ||
fill="#CBC8FF" | ||
/> | ||
</svg> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,7 @@ export const ModalServiceProvider: React.FC = ({ children }) => { | |
<Modal | ||
title={modalOptions.title} | ||
size={modalOptions.size} | ||
onClose={() => resultSubjectRef.current?.next({ type: "canceled" })} | ||
onClose={modalOptions.preventCancel ? undefined : () => resultSubjectRef.current?.next({ type: "canceled" })} | ||
> | ||
<modalOptions.content | ||
onCancel={() => resultSubjectRef.current?.next({ type: "canceled" })} | ||
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. The modal could still cancel here. Is this intended? 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. In general, our modals remain unchanged in that we should be able to cancel them by clicking "cancel" or pressing escape. Setting 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. I think if the implementation calls |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
@use "../../../scss/variables"; | ||
|
||
.modalContent { | ||
display: flex; | ||
flex-direction: column; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { useMemo } from "react"; | ||
import { FormattedMessage } from "react-intl"; | ||
|
||
import { Button } from "components"; | ||
|
||
import { AirbyteCatalog, CatalogDiff } from "core/request/AirbyteClient"; | ||
|
||
import { ModalBody, ModalFooter } from "../../../components/Modal"; | ||
import styles from "./CatalogDiffModal.module.scss"; | ||
import { DiffSection } from "./components/DiffSection"; | ||
import { FieldSection } from "./components/FieldSection"; | ||
import { getSortedDiff } from "./utils"; | ||
|
||
interface CatalogDiffModalProps { | ||
catalogDiff: CatalogDiff; | ||
catalog: AirbyteCatalog; | ||
onClose: () => void; | ||
} | ||
|
||
export const CatalogDiffModal: React.FC<CatalogDiffModalProps> = ({ catalogDiff, catalog, onClose }) => { | ||
edmundito marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { newItems, removedItems, changedItems } = useMemo( | ||
() => getSortedDiff(catalogDiff.transforms), | ||
[catalogDiff.transforms] | ||
); | ||
|
||
return ( | ||
<> | ||
<ModalBody maxHeight={400} padded={false}> | ||
<div className={styles.modalContent}> | ||
{removedItems.length > 0 && <DiffSection streams={removedItems} diffVerb="removed" catalog={catalog} />} | ||
{newItems.length > 0 && <DiffSection streams={newItems} diffVerb="new" />} | ||
{changedItems.length > 0 && <FieldSection streams={changedItems} diffVerb="changed" />} | ||
</div> | ||
</ModalBody> | ||
<ModalFooter> | ||
<Button onClick={() => onClose()}> | ||
<FormattedMessage id="connection.updateSchema.confirm" /> | ||
</Button> | ||
</ModalFooter> | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
@use "../../../../scss/variables"; | ||
@forward "../components/StreamRow.module.scss"; | ||
@forward "../components/DiffSection.module.scss"; | ||
|
||
.accordionContainer { | ||
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. Is DiffAccordion a unique design for the diff modal or is it a more generic component? 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. It's unique for this modal built on top of the Headless UI "disclosure" component. I didn't feel like there was enough I did on top of what Headless UI already provided to implement a generic Airbyte "Accordion", although that was my original plan. Happy to revisit the idea if it seems worthwhile. |
||
width: 100%; | ||
} | ||
|
||
.accordionButton { | ||
width: 100%; | ||
background: none; | ||
border: none; | ||
height: 40px; | ||
display: flex; | ||
flex-direction: row; | ||
align-items: center; | ||
justify-content: flex-start; | ||
padding: variables.$spacing-sm; | ||
font-weight: 400; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { Disclosure } from "@headlessui/react"; | ||
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 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. 🚀 |
||
import { useMemo } from "react"; | ||
|
||
import { StreamTransform } from "core/request/AirbyteClient"; | ||
|
||
import { getSortedDiff } from "../utils"; | ||
import styles from "./DiffAccordion.module.scss"; | ||
import { DiffAccordionHeader } from "./DiffAccordionHeader"; | ||
import { DiffFieldTable } from "./DiffFieldTable"; | ||
|
||
interface DiffAccordionProps { | ||
streamTransform: StreamTransform; | ||
} | ||
|
||
export const DiffAccordion: React.FC<DiffAccordionProps> = ({ streamTransform }) => { | ||
const { newItems, removedItems, changedItems } = useMemo( | ||
() => getSortedDiff(streamTransform.updateStream), | ||
[streamTransform.updateStream] | ||
); | ||
|
||
return ( | ||
<div className={styles.accordionContainer}> | ||
<Disclosure> | ||
{({ open }) => ( | ||
<> | ||
<Disclosure.Button className={styles.accordionButton}> | ||
<DiffAccordionHeader | ||
streamDescriptor={streamTransform.streamDescriptor} | ||
removedCount={removedItems.length} | ||
newCount={newItems.length} | ||
changedCount={changedItems.length} | ||
open={open} | ||
/> | ||
</Disclosure.Button> | ||
<Disclosure.Panel> | ||
{removedItems.length > 0 && <DiffFieldTable fieldTransforms={removedItems} diffVerb="removed" />} | ||
{newItems.length > 0 && <DiffFieldTable fieldTransforms={newItems} diffVerb="new" />} | ||
{changedItems.length > 0 && <DiffFieldTable fieldTransforms={changedItems} diffVerb="changed" />} | ||
</Disclosure.Panel> | ||
</> | ||
)} | ||
</Disclosure> | ||
</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.
Is
aria-label
right here? I believe that's what @timroes had suggestd for these, though I can't remember why that and notaria-description
. This isn't an interactive element.Good news in general: We can now start adding
aria-labels
other places that use this component as well (this is used to render logos on our list pages, the little circles with the numbers in it on list pages, etc.)