multi close (x) fixed #286
Conversation
|
@JJassonn69 is attempting to deploy a commit to the Livepeer Team on Vercel. A member of the Team first needs to authorize it. |
rozzrr
left a comment
rozzrr
left a comment
There was a problem hiding this comment.
good work! 🥇 ive ran this using my branch to see there is only one X present now. left a few comments, but generally just optional / discussion points that were probably implemented a long time ago!
| if (!latestTransaction || latestTransaction.step !== "summary") { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
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)
| import CloseIcon from "../../public/img/close.svg"; | ||
|
|
||
| const rotate = keyframes({ | ||
| "100%": { transform: "rotate(360deg)" }, |
There was a problem hiding this comment.
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.
| </Flex> | ||
|
|
||
| <Dialog | ||
| open={true} |
There was a problem hiding this comment.
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 👍
| > | ||
| Close | ||
| </Button> | ||
| <DialogClose asChild> |
There was a problem hiding this comment.
nice work here to wrap the close button 👍
|
@ECWireless since the codebase changed alot feel free to open a new pull request and make @JJassonn69 a co-author. |
3 participants
Annying double (X) in the dialog boxes when interacting with the contracts fixed.