Skip to content
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

feat(hackathon-surplus): show surplus modal when confirmation modal is closed #2773

Merged
merged 16 commits into from
Jul 5, 2023

Conversation

alfetopito
Copy link
Collaborator

Summary

Continuing #2752

Trigger surplus modal when order is fulfilled even if confirmation modal is closed

⚠️ Notes ⚠️

  • Styling not included, coming on feat: surplus modal #2767
  • Only tested SWAP, but should NOT trigger for LIMIT
  • I have not tested multiple orders fulfilling at the same time, might be an interesting edge case to check

To Test

  1. Place order
  2. Close confirmation modal
  • When fulfilled, modal should show up
  1. Place order
  2. Keep modal open
  • Surplus should be displayed
  1. Close modal
  • No other modals should be "hidden" underneath the first one

@vercel
Copy link

vercel bot commented Jun 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

🌃 Cosmos ↗︎

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactor

Comment on lines 28 to 35
<styledEl.Wrapper>
<styledEl.Section>
<styledEl.Header>
<styledEl.CloseIconWrapper onClick={onDismiss} />
</styledEl.Header>
</styledEl.Section>
<SurplusModal order={order} />
</styledEl.Wrapper>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re-using the same components to works nicely with #2767 without any changes

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

code review only, no test

src/common/pure/Modal/index.tsx Outdated Show resolved Hide resolved
@@ -23,6 +25,7 @@ export const SwapModals = React.memo(function (props: SwapModalsProps) {
{/*<CowSubsidyModal isOpen={showCowSubsidyModal} onDismiss={closeModals} /> */}
{<ConfirmSwapModalSetup {...confirmSwapProps} />}
{showNativeWrapModal && <EthFlowModal {...ethFlowProps} />}
{!showNativeWrapModal && <SurplusModalSetup />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this condition correct? !showNativeWrapModal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I just want to make sure it's not displayed by accident when EthFlowModal is active

const order = useOrder({ id: orderId, chainId })

const onDismiss = useCallback(() => {
orderId && removeOrderId(orderId)
Copy link
Contributor

Choose a reason for hiding this comment

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

cool how you handled the queue of modals

Copy link
Collaborator

@shoom3301 shoom3301 left a comment

Choose a reason for hiding this comment

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

Great changes!

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @alfetopito , great! I noticed that styles of the modal are a bit off, but you are aware of it.
image

I have also tested edge cases:

  1. 2 orders in a regular mode: the 1st progress bar is closed, the 2nd is opened: everything worked as expected. When the 1st order was filled, no Surplus modal appeared, but the progress bar changes the view when the 2nd order was filled.
  2. Expert mode, 2 orders: 2 modals appear one after another (or under another). So it looks like I need to press on the close icon twice in order to close the Surplus modal (however, I close 2 modals as numbers are changed when I close the 1st one)). Not sure if we need to fix this
  3. Regular mode, 2 orders with closed bars: the same behavior as in the p.2.
  4. When my Limit order is filled, I also see the surplus modal on the Swap page!
    image

I did not test it for TWAP order. However, let me know please if I need to.

@alfetopito alfetopito force-pushed the hackathon/surplus_modal-popup branch from da10508 to 6539aab Compare July 4, 2023 13:33
@alfetopito
Copy link
Collaborator Author

Hey @alfetopito , great! I noticed that styles of the modal are a bit off, but you are aware of it. image

I was able to fix it \o/

I have also tested edge cases:

1. 2 orders in a regular mode: the 1st progress bar is closed, the 2nd is opened: everything worked as expected. When the 1st order was filled, no Surplus modal appeared, but the progress bar changes the view when the 2nd order was filled.

2. Expert mode, 2 orders: 2 modals appear one after another (or under another). So it looks like I need to press on the close icon twice in order to close the Surplus modal (however, I close 2 modals as numbers are changed when I close the 1st one)). Not sure if we need to fix this

3. Regular mode, 2 orders with closed bars: the same behavior as in the p.2.

4. When my Limit order is filled, I also see the surplus modal on the Swap page!
   ![image](https://user-images.githubusercontent.com/70885163/250882579-3a4b08a8-9eb9-41e2-99e4-900c0352b2d0.png)

Now pop up should only show for swap orders

I did not test it for TWAP order. However, let me know please if I need to.

No need, if it works for limit (by not showing) it's enough

@elena-zh
Copy link

elena-zh commented Jul 4, 2023

Hey @alfetopito , great job!
One nitpick: can we wrap a token name by words in the modal?
image

@alfetopito
Copy link
Collaborator Author

alfetopito commented Jul 4, 2023

Hey @alfetopito , great job! One nitpick: can we wrap a token name by words in the modal? image

That, I'll delegate to @fairlighteth as it's out of my reach

@alfetopito alfetopito merged commit 645b069 into develop Jul 5, 2023
11 of 12 checks passed
@alfetopito alfetopito deleted the hackathon/surplus_modal-popup branch July 5, 2023 08:26
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2023
@elena-zh
Copy link

elena-zh commented Jul 5, 2023

That, I'll delegate to @fairlighteth as it's out of my reach

Hey @alfetopito , @fairlighteth , I have created #2790 issue for this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants