Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

switcher added #2013

Merged
merged 15 commits into from
Jan 24, 2022
Merged

switcher added #2013

merged 15 commits into from
Jan 24, 2022

Conversation

maria-vslvn
Copy link
Contributor

Summary

Fixes #578
Recipient mode switcher was added to the Swap page in Setting menu. When turned on, the user should see the icon (was used one for example) that indicates that the mod is turned on, also the link + Add a send (optional) should appear on the swap form just like it appears when Expert mode is on.

To Test

  1. Open the page Swap
  2. Open Settings
  3. Turn on the Recipient Mode
  4. Check out the indicator in the swap form header, checkout the link

@maria-vslvn
Copy link
Contributor Author

@ramirotw i'm not sure i know how to connect the recipient var of the state and our switcher states, as i don't know what the data should accept the recipient and how it should work exactly, maybe you could help me with this?:)

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@elena-zh
Copy link

HI @maria-vslvn !

As you are currently working with the switcher, might there be a possibility to fix the #1646 UI issue?

In regard to the current PR, I have to list come issues:

  1. I'm not sure that we need to call an additional confirmation modal to add the 'Recipient' field. @alfetopito , am I right?
  2. I have to notice, that it is not a 'mode': the toggle should be named as 'Toggle Recipient' and should only add/remove 'Add a send (optional)' field to the form.
  3. There is no need to show a 'cow' icon when the recipient toggle is switched on: the cow indicates Expert mode, but we should be able to add the field in the regular mode also.
    image
  4. Tooltip icon is shifted in a mobile view
    image
  5. As you have mentioned above, there is no 'Add send' field on the form when the toggle is switched on.
  6. We need to change the tooltip for the field... @alfetopito , could you please help with this?
    image

@ramirotw ramirotw added the Protofire Handled by Protofire development team label Dec 21, 2021
@ramirotw ramirotw force-pushed the feature/578-recipient-mode-switch branch from b2ab2e9 to f971dc8 Compare December 21, 2021 10:27
@alfetopito
Copy link
Contributor

HI @maria-vslvn !

As you are currently working with the switcher, might there be a possibility to fix the #1646 UI issue?

In regard to the current PR, I have to list come issues:

1. I'm not sure that we need to call an additional confirmation modal to add the 'Recipient' field. @alfetopito , am I right?

2. I have to notice, that it is not a 'mode': the toggle should be named as 'Toggle Recipient' and should only add/remove 'Add a send (optional)' field to the form.

3. There is no need to show a 'cow' icon when the recipient toggle is switched on: the cow indicates  Expert mode, but we should be able to add the field in the regular mode also.
   ![image](https://user-images.githubusercontent.com/70885163/146754403-4c69a376-8629-430f-b22d-189752761d58.png)

4. Tooltip icon is shifted in a mobile view
   ![image](https://user-images.githubusercontent.com/70885163/146754657-a89b5346-333c-4230-a8c9-2e99306643c7.png)

5. As you have mentioned above, there is no 'Add send' field on the form when the toggle is switched on.

6. We need to change the tooltip for the field... @alfetopito , could you please help with this?
   ![image](https://user-images.githubusercontent.com/70885163/146753741-539f3b70-095f-4e00-be92-33068cacd44c.png)

Agree with you Elena, there's no need to have a new icon.
The link to add a recipient is enough IMO

Also, it doesn't seem to work. I turned it on and I don't see the option to add a recipient, only when I turn expert mode on
Screenshot from 2021-12-21 07-46-05

Regarding the tooltip, something like:
"Allows you to choose a destination address for the swap other than the connected one"

@avsavsavs what do you think about that sentence?

@ramirotw
Copy link
Contributor

@maria-vslvn @elena-zh @alfetopito I have updated and simplify the workflow. Still pending the confirmation for the tooltip message and the css changes for mobile as Elena mentioned

@elena-zh
Copy link

@ramirotw , thanks!

A couple of new issues:

  1. It would be great to close the setting modal when toggle on/off an option
    image

  2. It would be nice to remove 'Add a send' field when Expert mode is on , and Recipient toggle is off. 'Add a send' field should appear when Recipient toggle is on with no regard to a mode.
    image

image

  1. It would be nice to make this message look nicer in a confirmation modal. @alongoni , could you please help with this?
    image

  2. It would be great to hide the Recipient field when we switch it off
    image

  3. Still need to change tooltip according to the provided above text.

@ramirotw
Copy link
Contributor

@elena-zh points 1, 2 and 4 change the current behavior as it is with the Expert Mode, since those changes weren't discussed in #578 I didn't include them. Regarding 2 and 4 I'm not sure if hiding the toggle in Expert mode is a good idea, users that already have the Expert Mode on will stop seeing the toggle in the swap screen without any alert. Those are small changes though, when confirmed we can update it

@alfetopito
Copy link
Contributor

Agree with Ramiro, let's keep the same behaviour for Expert mode.

One thing that we can do though, is to disable the recipient toggle when expert mode is on.

What do you think of that?

@elena-zh
Copy link

elena-zh commented Dec 21, 2021

@ramirotw ,
regarding to the case 1, yes, it exists on the Prod. So, @alfetopito , WDYT about this change? Should we leave it as it is, or we could close the settings modal as soon as we select an option in the toggle?

In regard to case 2, I think it was discussed here: #578 (comment) : to make a separate toggle that controls Add a send field.
Anyways, if my explanation is not good enough, I will add another argument to fix case 2: it looks a bit strange when 'Add Recipient' toggle is off, but the field is displayed on the form
image
Video: https://watch.screencastify.com/v/K53NKTtKkOQ2bultd3rX
So here we need whether to switch this toggle ON automatically when we switch the Expert mode on, or just implement the option I mentioned above. Again, @alfetopito , WDYT?

Case 4. I do not mean to hide the Expert mode toggle. I mean hide 'Add a send' field when we switch 'Toggle Recipient' off.

Please let me know if you have any questions.

Thanks.

@ramirotw
Copy link
Contributor

Case 4. I do not mean to hide the Expert mode toggle. I mean hide 'Add a send' field when we switch 'Toggle Recipient' off.

yes I understood that, currently that's how it works with Expert Mode on, if the user clicked the + Add a send (optional) and then switches off Expert Mode, the input field will still be visible until the page is refreshed

@ramirotw
Copy link
Contributor

One thing that we can do though, is to disable the recipient toggle when expert mode is on.

I like this, simple and we keep the current behavior

@elena-zh
Copy link

Case 4. I do not mean to hide the Expert mode toggle. I mean hide 'Add a send' field when we switch 'Toggle Recipient' off.

yes I understood that, currently that's how it works with Expert Mode on, if the user clicked the + Add a send (optional) and then switches off Expert Mode, the input field will still be visible until the page is refreshed

yes, #505 bug.
I thought it would be wise enough to fix all these issues when implementing changes with recipient toggle.

@ramirotw
Copy link
Contributor

yes, #505 bug.
I thought it would be wise enough to fix all these issues when implementing changes with recipient toggle.

great, haven't seen that one. Ok, I'll coordinate with @maria-vslvn so we can finish these:

  1. disable Toggle Recipient switcher when Expert Mode is on
  2. Fix Add recipient remains after disabling expert mode  #505
  3. Update tooltip
  4. Fix tooltip icon being shifted in mobile view

@elena-zh
Copy link

@ramirotw , cool, thank you!
Btw, regarding case 4: when the toggle was renamed, tooltip icon started to look good to me. Soo I have not mentioned this issue today. So if it looks the same to you, there is no need to fix it, I think.
image

Also, if you have a capacity, there are some additional issues on the board related to the current feature:
#1998 and #1216 .. So it would be super duper great to close all of them in one pr 😉

@ramirotw
Copy link
Contributor

Also, if you have a capacity, there are some additional issues on the board related to the current feature:
#1998 and #1216 .. So it would be super duper great to close all of them in one pr 😉

is the Swap anyway checkbox issue in #1998 related to this? I can close it, but not sure if it's related

@elena-zh
Copy link

@ramirotw , ooops.. #998, sorry about that

@alongoni
Copy link
Contributor

Hey @elena-zh and @maria-vslvn, regarding this point:

  1. It would be nice to make this message look nicer in a confirmation modal. @alongoni , could you please help with this?
    image

Maybe we can decrease the font-size and add some margin:
image

@elena-zh
Copy link

@alongoni , looks much better!
However, may be we could decrease this margin?
image

Also, we can make the test font in italic, and even smaller to correspond to the text above.
As you can see, the modal can be too long, that is why this ugly scroll bars appear (in Win10), so making the message look more compact can help to avoid this...
WDYT?

@alongoni
Copy link
Contributor

alongoni commented Dec 23, 2021

@alongoni , looks much better! However, may be we could decrease this margin?

Agree with this:
image

Also, we can make the test font in italic, and even smaller to correspond to the text above. As you can see, the modal can be too long, that is why this ugly scroll bars appear (in Win10), so making the message look more compact can help to avoid this... WDYT?

I'm not sure about bringing to the same level (talking visually) the "Output will be sent to ... " with the previous small text in italic. Mainly because in this step is important to review where the founds will be send. Other thing, I was thinking if we can rephrase "Output will be sent to ... " to "The funds will be set to ..." or something like that.

Regarding the scrollbar. We need to add some css to make it better. In a new issue I guess.
image
(Chrome browser on Linux)

@elena-zh
Copy link

Thanks @alongoni !
We already have an issue on the board related to the scrollbar in the confirmation modal, so I have updated its description #1145 to make this scrollbar look nicer.

@alongoni alongoni added the app:CowSwap CowSwap app label Jan 6, 2022
@anxolin
Copy link
Contributor

anxolin commented Jan 13, 2022

Should we finalize this PR?

@ramirotw
Copy link
Contributor

Should we finalize this PR?

just resolved some conflicts and fixed some issues, it should be ok to merge after passing QA

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

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.

Looks great! thanks for pushing this through the finish line

@elena-zh
Copy link

yes, #505 bug.
I thought it would be wise enough to fix all these issues when implementing changes with recipient toggle.

great, haven't seen that one. Ok, I'll coordinate with @maria-vslvn so we can finish these:

  1. disable Toggle Recipient switcher when Expert Mode is on
  2. Fix Add recipient remains after disabling expert mode  #505
  3. Update tooltip
  4. Fix tooltip icon being shifted in mobile view

Hey @ramirotw ! Great changes! However:1

  1. p2 (Add recipient remains after disabling expert mode  #505 issue) is still not fixed here
    505

  2. Once we disable recipient toggle in the expert mode, it would be nice to remove hovered effect from its buttons
    disabled

  3. Design changes from this comment were not applied in the latest commit

@ramirotw ramirotw linked an issue Jan 17, 2022 that may be closed by this pull request
@ramirotw
Copy link
Contributor

3. Design changes from this comment were not applied in the latest commit

@elena-zh please re-test. About 3 the only change I added was the removal of the top padding and nothing else. @alongoni wasn't quite happy about aligning the label with the previous one.

@elena-zh
Copy link

Hey @ramirotw , it might be a nitpick, but ON button still changes its color on hover. See the video:
https://watch.screencastify.com/v/0gH2FjP0L4yCcwlgystR
Maybe @alongoni could help you to resolve this?

Or let me know if I need to create a separate issue for this...

As for the p.3, I'd add a padding between the message and the Confirm button. Besides, I'd align the text with the rest text on the modal.
image
Compare with the design:
image

@ramirotw
Copy link
Contributor

Hey @ramirotw , it might be a nitpick, but ON button still changes its color on hover. See the video:
https://watch.screencastify.com/v/0gH2FjP0L4yCcwlgystR

Yes, I saw that but overriding that hover was cumbersome as the Toggle component was defined under the uniswap files and it would be an overkill to mod that file just to disable that darkened hover color. Another option is this:
image

Disabling both toggle buttons is an option, wdyt?

@elena-zh
Copy link

@ramirotw , thanks for your comments.
Let leave it as it is right now then. I will add it to the existing one #1646 , so we can fix this later.

And let wait for @alongoni review on the p.3

@alongoni
Copy link
Contributor

@elena-zh I've added some padding and now look like this:
image

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Great job!

@ramirotw ramirotw merged commit 2ffd5fe into develop Jan 24, 2022
@ramirotw ramirotw deleted the feature/578-recipient-mode-switch branch January 24, 2022 13:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:CowSwap CowSwap app Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Recipient switch in Settings Add recipient remains after disabling expert mode
6 participants