Skip to content

multi close (x) fixed #286

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

Open
JJassonn69 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

multi close (x) fixed #286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 19 additions & 32 deletions components/TxSummaryDialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import {
Box,
Button,
Dialog,
DialogClose,
DialogContent,
Flex,
keyframes,
Text,
} from "@livepeer/design-system";
import { sentenceCase } from "change-case";
import { useExplorerStore } from "hooks";
import CloseIcon from "../../public/img/close.svg";

const rotate = keyframes({
"100%": { transform: "rotate(360deg)" },
Copy link
Contributor

@rozzrr rozzrr Apr 8, 2025

Choose a reason for hiding this comment

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

summary could maybe live in a constant / enum if we expect further steps / change down the line. or if this needed elsewhere would make sense to have it set once.

Expand All @@ -23,29 +23,15 @@ const Index = () => {
}

Copy link
Contributor

@rozzrr rozzrr Apr 8, 2025

Choose a reason for hiding this comment

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

might be worth adding some comments above the if statement so its clear on why this is important / relevant to this dialog. (just for future devs like me to get a clearer view)

return (
<Dialog open={true}>
<DialogContent
css={{ minWidth: 370 }}
onPointerDownOutside={clearLatestTransaction}
>
<Flex
css={{
justifyContent: "flex-end",
}}
>
<Box
as={CloseIcon}
css={{
zIndex: 1,
right: 20,
cursor: "pointer",
top: 20,
color: "$white",
}}
onClick={clearLatestTransaction}
/>
</Flex>

<Dialog
open={true}
Copy link
Contributor

@rozzrr rozzrr Apr 8, 2025

Choose a reason for hiding this comment

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

dialog is always true, which is fine since we are controlling this via the if statement above, but as this isnt a "fully" controlled dialog - totally optional but we could have a boolean value which is determined such as isOpen = !!latestTransaction && ....stuff and then using the isOpen as the open state value. This way we avoid returning null and might be easier to extend if we introduce more complex scenarios down the line.

just a thought - current approch works fine too 👍

onOpenChange={(open) => {
if (!open) {
clearLatestTransaction();
}
}}
>
<DialogContent css={{ minWidth: 370 }}>
{latestTransaction?.error ? (
<Box />
) : (
Expand Down Expand Up @@ -92,14 +78,15 @@ const Index = () => {
? `${sentenceCase(latestTransaction?.error)}.`
: "Error with transaction, please check your inputs and try again."}
</Text>
<Button
onClick={clearLatestTransaction}
variant="primary"
size="4"
css={{ width: "100%" }}
>
Close
</Button>
<DialogClose asChild>
Copy link
Contributor

@rozzrr rozzrr Apr 8, 2025

Choose a reason for hiding this comment

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

nice work here to wrap the close button 👍

<Button
size="4"
variant="primary"
css={{ width: "100%" }}>

Close
</Button>
</DialogClose>
</>
) : (
<Text>Confirm latest transaction in your wallet.</Text>
Expand Down